diff mbox series

[v1] mkeficapsule: fix efi_firmware_management_capsule_header data type

Message ID 20230616082805.4652-1-stefan.herbrechtsmeier-oss@weidmueller.com
State Accepted, archived
Commit e05a4b12a056fd7fe22e85715c8f5e5e811fb551
Delegated to: Heinrich Schuchardt
Headers show
Series [v1] mkeficapsule: fix efi_firmware_management_capsule_header data type | expand

Commit Message

Stefan Herbrechtsmeier June 16, 2023, 8:28 a.m. UTC 
From: Malte Schmidt <malte.schmidt@weidmueller.com>

The data type of item_offset_list shall be UINT64 according to the UEFI [1]
specifications.

In include/efi_api.h the correct data type is used. The bug was probably
never noticed because of little endianness.

[1] https://uefi.org/specs/UEFI/2.10/index.html

Signed-off-by: Malte Schmidt <malte.schmidt@weidmueller.com>

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
---

 tools/eficapsule.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

AKASHI Takahiro June 17, 2023, 12:33 a.m. UTC | #1 
On Fri, Jun 16, 2023 at 10:28:05AM +0200, Stefan Herbrechtsmeier wrote:
> From: Malte Schmidt <malte.schmidt@weidmueller.com>
> 
> The data type of item_offset_list shall be UINT64 according to the UEFI [1]
> specifications.
> 
> In include/efi_api.h the correct data type is used. The bug was probably
> never noticed because of little endianness.

Thank you.
Just in case, tools/eficapsule.h has the same struct definition
because I didn't want to include U-Boot's corresponding header
to avoid including a conflicting header file.

Please fix it as well.

-Takahiro Akashi

> 
> [1] https://uefi.org/specs/UEFI/2.10/index.html
> 
> Signed-off-by: Malte Schmidt <malte.schmidt@weidmueller.com>
> 
> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
> ---
> 
>  tools/eficapsule.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/eficapsule.h b/tools/eficapsule.h
> index 753fb73313..2099a2e9b8 100644
> --- a/tools/eficapsule.h
> +++ b/tools/eficapsule.h
> @@ -63,7 +63,7 @@ struct efi_firmware_management_capsule_header {
>  	uint32_t version;
>  	uint16_t embedded_driver_count;
>  	uint16_t payload_item_count;
> -	uint32_t item_offset_list[];
> +	uint64_t item_offset_list[];
>  } __packed;
>  
>  /* image_capsule_support */
> -- 
> 2.30.2
>
Schmidt, Malte June 19, 2023, 9:45 a.m. UTC | #2 
Hello Takahir,

Am 17.06.2023 um 02:33 schrieb AKASHI Takahiro:
> On Fri, Jun 16, 2023 at 10:28:05AM +0200, Stefan Herbrechtsmeier wrote:
>> From: Malte Schmidt <malte.schmidt@weidmueller.com>
>>
>> The data type of item_offset_list shall be UINT64 according to the UEFI [1]
>> specifications.
>>
>> In include/efi_api.h the correct data type is used. The bug was probably
>> never noticed because of little endianness.
> Thank you.
> Just in case, tools/eficapsule.h has the same struct definition
> because I didn't want to include U-Boot's corresponding header
> to avoid including a conflicting header file.
>
> Please fix it as well.
>
> -Takahiro Akashi
Which header should I fix additionally? In include/efi_api.h the correct
type is used, so there is nothing to fix.

Best Regards
Malte
>> [1] https://uefi.org/specs/UEFI/2.10/index.html
>>
>> Signed-off-by: Malte Schmidt <malte.schmidt@weidmueller.com>
>>
>> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>> ---
>>
>>   tools/eficapsule.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/eficapsule.h b/tools/eficapsule.h
>> index 753fb73313..2099a2e9b8 100644
>> --- a/tools/eficapsule.h
>> +++ b/tools/eficapsule.h
>> @@ -63,7 +63,7 @@ struct efi_firmware_management_capsule_header {
>>   	uint32_t version;
>>   	uint16_t embedded_driver_count;
>>   	uint16_t payload_item_count;
>> -	uint32_t item_offset_list[];
>> +	uint64_t item_offset_list[];
>>   } __packed;
>>   
>>   /* image_capsule_support */
>> -- 
>> 2.30.2
>>
AKASHI Takahiro June 19, 2023, 9:53 a.m. UTC | #3 
On Mon, Jun 19, 2023 at 11:45:49AM +0200, Schmidt, Malte wrote:
> Hello Takahir,
> 
> Am 17.06.2023 um 02:33 schrieb AKASHI Takahiro:
> > On Fri, Jun 16, 2023 at 10:28:05AM +0200, Stefan Herbrechtsmeier wrote:
> > > From: Malte Schmidt <malte.schmidt@weidmueller.com>
> > > 
> > > The data type of item_offset_list shall be UINT64 according to the UEFI [1]
> > > specifications.
> > > 
> > > In include/efi_api.h the correct data type is used. The bug was probably
> > > never noticed because of little endianness.
> > Thank you.
> > Just in case, tools/eficapsule.h has the same struct definition
> > because I didn't want to include U-Boot's corresponding header
> > to avoid including a conflicting header file.
> > 
> > Please fix it as well.
> > 
> > -Takahiro Akashi
> Which header should I fix additionally? In include/efi_api.h the correct
> type is used, so there is nothing to fix.

That is tools/eficapsule.h

-Takahiro Akashi


> Best Regards
> Malte
> > > [1] https://uefi.org/specs/UEFI/2.10/index.html
> > > 
> > > Signed-off-by: Malte Schmidt <malte.schmidt@weidmueller.com>
> > > 
> > > Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
> > > ---
> > > 
> > >   tools/eficapsule.h | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/eficapsule.h b/tools/eficapsule.h
> > > index 753fb73313..2099a2e9b8 100644
> > > --- a/tools/eficapsule.h
> > > +++ b/tools/eficapsule.h
> > > @@ -63,7 +63,7 @@ struct efi_firmware_management_capsule_header {
> > >   	uint32_t version;
> > >   	uint16_t embedded_driver_count;
> > >   	uint16_t payload_item_count;
> > > -	uint32_t item_offset_list[];
> > > +	uint64_t item_offset_list[];
> > >   } __packed;
> > >   /* image_capsule_support */
> > > -- 
> > > 2.30.2
> > > 
>
Schmidt, Malte June 19, 2023, 10:37 a.m. UTC | #4 
Hello Takahiro,

Am 19.06.2023 um 11:53 schrieb AKASHI Takahiro:
> On Mon, Jun 19, 2023 at 11:45:49AM +0200, Schmidt, Malte wrote:
>> Hello Takahir,
>>
>> Am 17.06.2023 um 02:33 schrieb AKASHI Takahiro:
>>> On Fri, Jun 16, 2023 at 10:28:05AM +0200, Stefan Herbrechtsmeier wrote:
>>>> From: Malte Schmidt <malte.schmidt@weidmueller.com>
>>>>
>>>> The data type of item_offset_list shall be UINT64 according to the UEFI [1]
>>>> specifications.
>>>>
>>>> In include/efi_api.h the correct data type is used. The bug was probably
>>>> never noticed because of little endianness.
>>> Thank you.
>>> Just in case, tools/eficapsule.h has the same struct definition
>>> because I didn't want to include U-Boot's corresponding header
>>> to avoid including a conflicting header file.
>>>
>>> Please fix it as well.
>>>
>>> -Takahiro Akashi
>> Which header should I fix additionally? In include/efi_api.h the correct
>> type is used, so there is nothing to fix.
> That is tools/eficapsule.h
>
> -Takahiro Akashi
tools/eficapsule is the file to which my patch applies to. Please have 
another
look at it. Otherwise, please explain a bit more detailed what your are 
suggesting,
as I am a bit confused about what your point is.

Best Regards
Malte
>
>> Best Regards
>> Malte
>>>> [1] https://uefi.org/specs/UEFI/2.10/index.html
>>>>
>>>> Signed-off-by: Malte Schmidt <malte.schmidt@weidmueller.com>
>>>>
>>>> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>>>> ---
>>>>
>>>>    tools/eficapsule.h | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/eficapsule.h b/tools/eficapsule.h
>>>> index 753fb73313..2099a2e9b8 100644
>>>> --- a/tools/eficapsule.h
>>>> +++ b/tools/eficapsule.h
>>>> @@ -63,7 +63,7 @@ struct efi_firmware_management_capsule_header {
>>>>    	uint32_t version;
>>>>    	uint16_t embedded_driver_count;
>>>>    	uint16_t payload_item_count;
>>>> -	uint32_t item_offset_list[];
>>>> +	uint64_t item_offset_list[];
>>>>    } __packed;
>>>>    /* image_capsule_support */
>>>> -- 
>>>> 2.30.2
>>>>
AKASHI Takahiro June 19, 2023, 10:45 a.m. UTC | #5 
On Mon, Jun 19, 2023 at 12:37:48PM +0200, Schmidt, Malte wrote:
> Hello Takahiro,
> 
> Am 19.06.2023 um 11:53 schrieb AKASHI Takahiro:
> > On Mon, Jun 19, 2023 at 11:45:49AM +0200, Schmidt, Malte wrote:
> > > Hello Takahir,
> > > 
> > > Am 17.06.2023 um 02:33 schrieb AKASHI Takahiro:
> > > > On Fri, Jun 16, 2023 at 10:28:05AM +0200, Stefan Herbrechtsmeier wrote:
> > > > > From: Malte Schmidt <malte.schmidt@weidmueller.com>
> > > > > 
> > > > > The data type of item_offset_list shall be UINT64 according to the UEFI [1]
> > > > > specifications.
> > > > > 
> > > > > In include/efi_api.h the correct data type is used. The bug was probably
> > > > > never noticed because of little endianness.
> > > > Thank you.
> > > > Just in case, tools/eficapsule.h has the same struct definition
> > > > because I didn't want to include U-Boot's corresponding header
> > > > to avoid including a conflicting header file.
> > > > 
> > > > Please fix it as well.
> > > > 
> > > > -Takahiro Akashi
> > > Which header should I fix additionally? In include/efi_api.h the correct
> > > type is used, so there is nothing to fix.
> > That is tools/eficapsule.h
> > 
> > -Takahiro Akashi
> tools/eficapsule is the file to which my patch applies to. Please have
> another
> look at it. Otherwise, please explain a bit more detailed what your are
> suggesting,
> as I am a bit confused about what your point is.

Right, it's my misunderstanding.

-Takahiro Akashi

> Best Regards
> Malte
> > 
> > > Best Regards
> > > Malte
> > > > > [1] https://uefi.org/specs/UEFI/2.10/index.html
> > > > > 
> > > > > Signed-off-by: Malte Schmidt <malte.schmidt@weidmueller.com>
> > > > > 
> > > > > Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
> > > > > ---
> > > > > 
> > > > >    tools/eficapsule.h | 2 +-
> > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/tools/eficapsule.h b/tools/eficapsule.h
> > > > > index 753fb73313..2099a2e9b8 100644
> > > > > --- a/tools/eficapsule.h
> > > > > +++ b/tools/eficapsule.h
> > > > > @@ -63,7 +63,7 @@ struct efi_firmware_management_capsule_header {
> > > > >    	uint32_t version;
> > > > >    	uint16_t embedded_driver_count;
> > > > >    	uint16_t payload_item_count;
> > > > > -	uint32_t item_offset_list[];
> > > > > +	uint64_t item_offset_list[];
> > > > >    } __packed;
> > > > >    /* image_capsule_support */
> > > > > -- 
> > > > > 2.30.2
> > > > > 
>
Heinrich Schuchardt July 9, 2023, 8:31 a.m. UTC | #6 
On 6/16/23 10:28, Stefan Herbrechtsmeier wrote:
> From: Malte Schmidt <malte.schmidt@weidmueller.com>
>
> The data type of item_offset_list shall be UINT64 according to the UEFI [1]
> specifications.
>
> In include/efi_api.h the correct data type is used. The bug was probably
> never noticed because of little endianness.
>
> [1] https://uefi.org/specs/UEFI/2.10/index.html
>
> Signed-off-by: Malte Schmidt <malte.schmidt@weidmueller.com>
>
> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
> ---
>
>   tools/eficapsule.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/eficapsule.h b/tools/eficapsule.h
> index 753fb73313..2099a2e9b8 100644
> --- a/tools/eficapsule.h
> +++ b/tools/eficapsule.h
> @@ -63,7 +63,7 @@ struct efi_firmware_management_capsule_header {
>   	uint32_t version;
>   	uint16_t embedded_driver_count;
>   	uint16_t payload_item_count;
> -	uint32_t item_offset_list[];
> +	uint64_t item_offset_list[];

Defining the same structure in two places it bad practice.
https://source.denx.de/u-boot/custodians/u-boot-efi/-/issues/11

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

>   } __packed;
>
>   /* image_capsule_support */
AKASHI Takahiro July 10, 2023, 6:26 a.m. UTC | #7 
On Sun, Jul 09, 2023 at 10:31:55AM +0200, Heinrich Schuchardt wrote:
> On 6/16/23 10:28, Stefan Herbrechtsmeier wrote:
> > From: Malte Schmidt <malte.schmidt@weidmueller.com>
> > 
> > The data type of item_offset_list shall be UINT64 according to the UEFI [1]
> > specifications.
> > 
> > In include/efi_api.h the correct data type is used. The bug was probably
> > never noticed because of little endianness.
> > 
> > [1] https://uefi.org/specs/UEFI/2.10/index.html
> > 
> > Signed-off-by: Malte Schmidt <malte.schmidt@weidmueller.com>
> > 
> > Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
> > ---
> > 
> >   tools/eficapsule.h | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/eficapsule.h b/tools/eficapsule.h
> > index 753fb73313..2099a2e9b8 100644
> > --- a/tools/eficapsule.h
> > +++ b/tools/eficapsule.h
> > @@ -63,7 +63,7 @@ struct efi_firmware_management_capsule_header {
> >   	uint32_t version;
> >   	uint16_t embedded_driver_count;
> >   	uint16_t payload_item_count;
> > -	uint32_t item_offset_list[];
> > +	uint64_t item_offset_list[];
> 
> Defining the same structure in two places it bad practice.
> https://source.denx.de/u-boot/custodians/u-boot-efi/-/issues/11

I had a good reason for adding a tool-specific header, instead of
using headers under 'include' dir, when I posted v7 of "efi_loader:
capsule: improve capsule authentication support" patch.
The cover letter says,
===
v7 (Nov 16, 2021)
  ...
* define eficapsule.h and include it from mkeficapsule (patch#3)
  Hopefully, the tool can now compile on non-linux host.
===

If I correctly remember, this reflects the comment below and the 
succeeding discussions:
https://lists.denx.de/pipermail/u-boot/2021-November/465859.html

-Takahiro Akashi


> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
> >   } __packed;
> > 
> >   /* image_capsule_support */
>
Heinrich Schuchardt July 10, 2023, 6:41 a.m. UTC | #8 
Am 10. Juli 2023 08:26:37 MESZ schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
>On Sun, Jul 09, 2023 at 10:31:55AM +0200, Heinrich Schuchardt wrote:
>> On 6/16/23 10:28, Stefan Herbrechtsmeier wrote:
>> > From: Malte Schmidt <malte.schmidt@weidmueller.com>
>> > 
>> > The data type of item_offset_list shall be UINT64 according to the UEFI [1]
>> > specifications.
>> > 
>> > In include/efi_api.h the correct data type is used. The bug was probably
>> > never noticed because of little endianness.
>> > 
>> > [1] https://uefi.org/specs/UEFI/2.10/index.html
>> > 
>> > Signed-off-by: Malte Schmidt <malte.schmidt@weidmueller.com>
>> > 
>> > Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>> > ---
>> > 
>> >   tools/eficapsule.h | 2 +-
>> >   1 file changed, 1 insertion(+), 1 deletion(-)
>> > 
>> > diff --git a/tools/eficapsule.h b/tools/eficapsule.h
>> > index 753fb73313..2099a2e9b8 100644
>> > --- a/tools/eficapsule.h
>> > +++ b/tools/eficapsule.h
>> > @@ -63,7 +63,7 @@ struct efi_firmware_management_capsule_header {
>> >   	uint32_t version;
>> >   	uint16_t embedded_driver_count;
>> >   	uint16_t payload_item_count;
>> > -	uint32_t item_offset_list[];
>> > +	uint64_t item_offset_list[];
>> 
>> Defining the same structure in two places it bad practice.
>> https://source.denx.de/u-boot/custodians/u-boot-efi/-/issues/11
>
>I had a good reason for adding a tool-specific header, instead of
>using headers under 'include' dir, when I posted v7 of "efi_loader:
>capsule: improve capsule authentication support" patch.
>The cover letter says,
>===
>v7 (Nov 16, 2021)
>  ...
>* define eficapsule.h and include it from mkeficapsule (patch#3)
>  Hopefully, the tool can now compile on non-linux host.
>===
>
>If I correctly remember, this reflects the comment below and the 
>succeeding discussions:
>https://lists.denx.de/pipermail/u-boot/2021-November/465859.html
>
>-Takahiro Akashi

We should be able to factor out a common header file which contains capsule specific structures.

I understand that we have to test building on BSD.

Best regards

Heinrich

>
>
>> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> 
>> >   } __packed;
>> > 
>> >   /* image_capsule_support */
>>
diff mbox series

Patch

diff --git a/tools/eficapsule.h b/tools/eficapsule.h
index 753fb73313..2099a2e9b8 100644
--- a/tools/eficapsule.h
+++ b/tools/eficapsule.h
@@ -63,7 +63,7 @@  struct efi_firmware_management_capsule_header {
 	uint32_t version;
 	uint16_t embedded_driver_count;
 	uint16_t payload_item_count;
-	uint32_t item_offset_list[];
+	uint64_t item_offset_list[];
 } __packed;
 
 /* image_capsule_support */