[U-Boot,v3,03/21] efi: add some missing __packed

Message ID 20170913220546.19560-4-robdclark@gmail.com
State Accepted
Delegated to: Alexander Graf
Headers show
Series
  • efi_loader: enough UEFI for standard distro boot
Related show

Commit Message

Rob Clark Sept. 13, 2017, 10:05 p.m.
All of the device-path related structures should be packed.  UEFI
defines the device-path as a byte-aligned data structure.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 include/efi_api.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Heinrich Schuchardt Sept. 15, 2017, 6:53 p.m. | #1
On 09/14/2017 12:05 AM, Rob Clark wrote:
> All of the device-path related structures should be packed.  UEFI
> defines the device-path as a byte-aligned data structure.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  include/efi_api.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/efi_api.h b/include/efi_api.h
> index ec1b321e8e..175341348e 100644
> --- a/include/efi_api.h
> +++ b/include/efi_api.h
> @@ -284,11 +284,11 @@ struct efi_device_path {
>  	u8 type;
>  	u8 sub_type;
>  	u16 length;
> -};
> +} __packed;
>  
>  struct efi_mac_addr {
>  	u8 addr[32];

Hello Alex, hello Rob,

32 bytes is more than the 6 bytes than we need.
Why was the structure defined this way?

> -};
> +} __packed;
>  
>  #define DEVICE_PATH_TYPE_MESSAGING_DEVICE	0x03
>  #  define DEVICE_PATH_SUB_TYPE_MSG_MAC_ADDR	0x0b
> @@ -297,7 +297,7 @@ struct efi_device_path_mac_addr {
>  	struct efi_device_path dp;
>  	struct efi_mac_addr mac;
>  	u8 if_type;
> -};
> +} __packed;
>  
>  #define DEVICE_PATH_TYPE_MEDIA_DEVICE		0x04
>  #  define DEVICE_PATH_SUB_TYPE_FILE_PATH	0x04
> @@ -305,7 +305,7 @@ struct efi_device_path_mac_addr {
>  struct efi_device_path_file_path {
>  	struct efi_device_path dp;
>  	u16 str[32];

Where do we get this 32 from?
Couldn't file/directory names be shorter (1 character) or longer (255
characters)?

Regards

Heinrich

> -};
> +} __packed;
>  
>  #define BLOCK_IO_GUID \
>  	EFI_GUID(0x964e5b21, 0x6459, 0x11d2, \
>
Rob Clark Sept. 15, 2017, 6:58 p.m. | #2
On Fri, Sep 15, 2017 at 2:53 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 09/14/2017 12:05 AM, Rob Clark wrote:
>> All of the device-path related structures should be packed.  UEFI
>> defines the device-path as a byte-aligned data structure.
>>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> ---
>>  include/efi_api.h | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/efi_api.h b/include/efi_api.h
>> index ec1b321e8e..175341348e 100644
>> --- a/include/efi_api.h
>> +++ b/include/efi_api.h
>> @@ -284,11 +284,11 @@ struct efi_device_path {
>>       u8 type;
>>       u8 sub_type;
>>       u16 length;
>> -};
>> +} __packed;
>>
>>  struct efi_mac_addr {
>>       u8 addr[32];
>
> Hello Alex, hello Rob,
>
> 32 bytes is more than the 6 bytes than we need.
> Why was the structure defined this way?
>

Off the top of my head, I'm not sure where that came from, maybe this
dp node is meant to be variable length?  But I guess that is also a
separate patch.

>> -};
>> +} __packed;
>>
>>  #define DEVICE_PATH_TYPE_MESSAGING_DEVICE    0x03
>>  #  define DEVICE_PATH_SUB_TYPE_MSG_MAC_ADDR  0x0b
>> @@ -297,7 +297,7 @@ struct efi_device_path_mac_addr {
>>       struct efi_device_path dp;
>>       struct efi_mac_addr mac;
>>       u8 if_type;
>> -};
>> +} __packed;
>>
>>  #define DEVICE_PATH_TYPE_MEDIA_DEVICE                0x04
>>  #  define DEVICE_PATH_SUB_TYPE_FILE_PATH     0x04
>> @@ -305,7 +305,7 @@ struct efi_device_path_mac_addr {
>>  struct efi_device_path_file_path {
>>       struct efi_device_path dp;
>>       u16 str[32];
>
> Where do we get this 32 from?
> Couldn't file/directory names be shorter (1 character) or longer (255
> characters)?
>

yup, file-path nodes should be variable length (and are after my
patches.. I have a later one that changes it to 'u16 str[]')

I believe the hard-coded 32 is due to current staticly initialized
device-path structs which go away with "efi_loader: refactor boot
device and loaded_image handling"

BR,
-R
Alexander Graf Sept. 21, 2017, 7:04 a.m. | #3
> All of the device-path related structures should be packed.  UEFI
> defines the device-path as a byte-aligned data structure.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>

Thanks, applied to efi-next

Alex
Peter Jones Oct. 3, 2017, 2:34 p.m. | #4
On Fri, Sep 15, 2017 at 06:53:39PM +0000, Heinrich Schuchardt wrote:
> On 09/14/2017 12:05 AM, Rob Clark wrote:
> > All of the device-path related structures should be packed.  UEFI
> > defines the device-path as a byte-aligned data structure.
> > 
> > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > ---
> >  include/efi_api.h | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/efi_api.h b/include/efi_api.h
> > index ec1b321e8e..175341348e 100644
> > --- a/include/efi_api.h
> > +++ b/include/efi_api.h
> > @@ -284,11 +284,11 @@ struct efi_device_path {
> >  	u8 type;
> >  	u8 sub_type;
> >  	u16 length;
> > -};
> > +} __packed;
> >  
> >  struct efi_mac_addr {
> >  	u8 addr[32];
> 
> Hello Alex, hello Rob,
> 
> 32 bytes is more than the 6 bytes than we need.
> Why was the structure defined this way?

It probably *should* have been variable length in the spec, but it is
defined as 32-bytes.  But 6 is also not correct - it's right for
ethernet, but not for everything.  I suspect they picked the length of
an IPoIB (Infiniband) MAC here.

> 
> > -};
> > +} __packed;
> >  
> >  #define DEVICE_PATH_TYPE_MESSAGING_DEVICE	0x03
> >  #  define DEVICE_PATH_SUB_TYPE_MSG_MAC_ADDR	0x0b
> > @@ -297,7 +297,7 @@ struct efi_device_path_mac_addr {
> >  	struct efi_device_path dp;
> >  	struct efi_mac_addr mac;
> >  	u8 if_type;
> > -};
> > +} __packed;
> >  
> >  #define DEVICE_PATH_TYPE_MEDIA_DEVICE		0x04
> >  #  define DEVICE_PATH_SUB_TYPE_FILE_PATH	0x04
> > @@ -305,7 +305,7 @@ struct efi_device_path_mac_addr {
> >  struct efi_device_path_file_path {
> >  	struct efi_device_path dp;
> >  	u16 str[32];
> 
> Where do we get this 32 from?
> Couldn't file/directory names be shorter (1 character) or longer (255
> characters)?

Note there's already another patch in this series fixing this.

Patch

diff --git a/include/efi_api.h b/include/efi_api.h
index ec1b321e8e..175341348e 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -284,11 +284,11 @@  struct efi_device_path {
 	u8 type;
 	u8 sub_type;
 	u16 length;
-};
+} __packed;
 
 struct efi_mac_addr {
 	u8 addr[32];
-};
+} __packed;
 
 #define DEVICE_PATH_TYPE_MESSAGING_DEVICE	0x03
 #  define DEVICE_PATH_SUB_TYPE_MSG_MAC_ADDR	0x0b
@@ -297,7 +297,7 @@  struct efi_device_path_mac_addr {
 	struct efi_device_path dp;
 	struct efi_mac_addr mac;
 	u8 if_type;
-};
+} __packed;
 
 #define DEVICE_PATH_TYPE_MEDIA_DEVICE		0x04
 #  define DEVICE_PATH_SUB_TYPE_FILE_PATH	0x04
@@ -305,7 +305,7 @@  struct efi_device_path_mac_addr {
 struct efi_device_path_file_path {
 	struct efi_device_path dp;
 	u16 str[32];
-};
+} __packed;
 
 #define BLOCK_IO_GUID \
 	EFI_GUID(0x964e5b21, 0x6459, 0x11d2, \