Re: [U-Boot] [PATCH 1/2] efi_loader: initialize ethernet device path fully

[Heinrich Schuchardt]

On 03/25/2018 10:11 PM, Heinrich Schuchardt wrote:
> On 03/25/2018 07:31 PM, Patrick Wildt wrote:
>> On Fri, Mar 23, 2018 at 07:58:07PM +0100, Heinrich Schuchardt wrote:
>>> On 03/23/2018 07:54 PM, Heinrich Schuchardt wrote:
 >From c38a011f699cf5a41b48c6fc547f3d0dde3d7cf9 Mon Sep 17 00:00:00 2001
 From: Patrick Wildt 
 Date: Fri, 23 Mar 2018 15:36:58 +0100
 Subject: [PATCH 1/2] efi_loader: initialize ethernet device path fully

 Since the backing memory for a new device path can contain stale
 data we have to make sure that we either zero the buffer or that
 we initialize all variables in the overlaying structure.  This
 fixes setting the boot device path in the loaded image protocol
 in case we boot from network.>
 Signed-off-by: Patrick Wildt 
 ---

> 
>>
>>>
 +  ndp->if_type = 1;
>>>
>>> This line does not give any clue what 1 relates to. Please, define a
>>> constant. EDK uses NET_IFTYPE_ETHERNET = 1.
>>>
>>> The UEFI spec refers to RFC3232. But I could not find the corresponding
>>> definition there.
>>> https://www.iana.org/assignments/ianaiftype-mib/ianaiftype-mib uses the
>>> value 6 for Ethernet interfaces.
>>>
>>> Please, provide the RFC or IANA page from which you have taken the value
>>> 1 in a comment for the constant.
>>
>> I guess I should have added /* Ethernet */ as comment, which would be as
>> precise information as given in the function dp_fill(void) where this
>> was introduced as part of commit 9dfd84da8ce :
>>
>> +/* Ethernet */
>> +dp->if_type = 1;
> 
> You got me. We should use the same constant here, too. I still do not
> know in which spec this value of 1 is really defined.
> 
> Best regards
> 
> Heinrich
> 

The UEFI spec 2.7 says for IfType:
Network interface type(i.e., 802.3, FDDI). See RFC 3232

RFC 3232 says that we should use the numbers published on www.iana.org.

https://www.iana.org/assignments/ianaiftype-mib/ianaiftype-mib has these
values for ifType

other: 1
ethernetCsmacd: 6 for all ethernet-like interfaces, as per RFC 3635

For converting device paths the UEFI specs requires:
"If IfType  is 0 or 1, then the MacAddr must be exactly six bytes."

For compatibility's sake we should stick with the value 1. But, please,
define a constant and use it wherever iftype is set.

Best regards

Heinrich
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/2] efi_loader: initialize ethernet device path fully

[Heinrich Schuchardt]

On 03/25/2018 07:31 PM, Patrick Wildt wrote:
> On Fri, Mar 23, 2018 at 07:58:07PM +0100, Heinrich Schuchardt wrote:
>> On 03/23/2018 07:54 PM, Heinrich Schuchardt wrote:
>>> >From c38a011f699cf5a41b48c6fc547f3d0dde3d7cf9 Mon Sep 17 00:00:00 2001
>>> From: Patrick Wildt 
>>> Date: Fri, 23 Mar 2018 15:36:58 +0100
>>> Subject: [PATCH 1/2] efi_loader: initialize ethernet device path fully
>>>
>>> Since the backing memory for a new device path can contain stale
>>> data we have to make sure that we either zero the buffer or that
>>> we initialize all variables in the overlaying structure.  This
>>> fixes setting the boot device path in the loaded image protocol
>>> in case we boot from network.>
>>> Signed-off-by: Patrick Wildt 
>>> ---
>>>  lib/efi_loader/efi_device_path.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/efi_loader/efi_device_path.c
>>> b/lib/efi_loader/efi_device_path.c
>>> index 3c735e60d3..a086c7bbd4 100644
>>> --- a/lib/efi_loader/efi_device_path.c
>>> +++ b/lib/efi_loader/efi_device_path.c
>>> @@ -777,7 +777,9 @@ struct efi_device_path *efi_dp_from_eth(void)
>>> ndp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
>>> ndp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_MAC_ADDR;
>>> ndp->dp.length = sizeof(*ndp);
>>> -   memcpy(ndp->mac.addr, eth_get_ethaddr(), ARP_HLEN);
>>> +   memset(>mac, 0, sizeof(ndp->mac));
>>> +   memcpy(>mac, eth_get_ethaddr(), ARP_HLEN);
>>
>> I would prefer if you would set the memory to zero in dp_alloc(). This
>> would avoid the observed problem in all device paths.
> 
> Sure, I will send another patch to the list which only changes
> dp_alloc() to zero the memory.

I already reviewed that one. Thanks.

> 
>>
>>> +   ndp->if_type = 1;
>>
>> This line does not give any clue what 1 relates to. Please, define a
>> constant. EDK uses NET_IFTYPE_ETHERNET = 1.
>>
>> The UEFI spec refers to RFC3232. But I could not find the corresponding
>> definition there.
>> https://www.iana.org/assignments/ianaiftype-mib/ianaiftype-mib uses the
>> value 6 for Ethernet interfaces.
>>
>> Please, provide the RFC or IANA page from which you have taken the value
>> 1 in a comment for the constant.
> 
> I guess I should have added /* Ethernet */ as comment, which would be as
> precise information as given in the function dp_fill(void) where this
> was introduced as part of commit 9dfd84da8ce :
> 
> + /* Ethernet */
> + dp->if_type = 1;

You got me. We should use the same constant here, too. I still do not
know in which spec this value of 1 is really defined.

Best regards

Heinrich

> 
> I don't know if setting this even matters, I added it for consistency,
> but left out the comment.
> 
> Best regards,
> Patrick
> 
>> Best regards
>>
>> Heinrich
>>
>>> buf = [1];
>>>
>>> *((struct efi_device_path *)buf) = END;
>>>
>>
> 

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/2] efi_loader: initialize ethernet device path fully

[Patrick Wildt]

On Fri, Mar 23, 2018 at 07:58:07PM +0100, Heinrich Schuchardt wrote:
> On 03/23/2018 07:54 PM, Heinrich Schuchardt wrote:
> >>From c38a011f699cf5a41b48c6fc547f3d0dde3d7cf9 Mon Sep 17 00:00:00 2001
> > From: Patrick Wildt 
> > Date: Fri, 23 Mar 2018 15:36:58 +0100
> > Subject: [PATCH 1/2] efi_loader: initialize ethernet device path fully
> > 
> > Since the backing memory for a new device path can contain stale
> > data we have to make sure that we either zero the buffer or that
> > we initialize all variables in the overlaying structure.  This
> > fixes setting the boot device path in the loaded image protocol
> > in case we boot from network.>
> > Signed-off-by: Patrick Wildt 
> > ---
> >  lib/efi_loader/efi_device_path.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/efi_loader/efi_device_path.c
> > b/lib/efi_loader/efi_device_path.c
> > index 3c735e60d3..a086c7bbd4 100644
> > --- a/lib/efi_loader/efi_device_path.c
> > +++ b/lib/efi_loader/efi_device_path.c
> > @@ -777,7 +777,9 @@ struct efi_device_path *efi_dp_from_eth(void)
> > ndp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
> > ndp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_MAC_ADDR;
> > ndp->dp.length = sizeof(*ndp);
> > -   memcpy(ndp->mac.addr, eth_get_ethaddr(), ARP_HLEN);
> > +   memset(>mac, 0, sizeof(ndp->mac));
> > +   memcpy(>mac, eth_get_ethaddr(), ARP_HLEN);
> 
> I would prefer if you would set the memory to zero in dp_alloc(). This
> would avoid the observed problem in all device paths.

Sure, I will send another patch to the list which only changes
dp_alloc() to zero the memory.

> 
> > +   ndp->if_type = 1;
> 
> This line does not give any clue what 1 relates to. Please, define a
> constant. EDK uses NET_IFTYPE_ETHERNET = 1.
> 
> The UEFI spec refers to RFC3232. But I could not find the corresponding
> definition there.
> https://www.iana.org/assignments/ianaiftype-mib/ianaiftype-mib uses the
> value 6 for Ethernet interfaces.
> 
> Please, provide the RFC or IANA page from which you have taken the value
> 1 in a comment for the constant.

I guess I should have added /* Ethernet */ as comment, which would be as
precise information as given in the function dp_fill(void) where this
was introduced as part of commit 9dfd84da8ce :

+   /* Ethernet */
+   dp->if_type = 1;

I don't know if setting this even matters, I added it for consistency,
but left out the comment.

Best regards,
Patrick

> Best regards
> 
> Heinrich
> 
> > buf = [1];
> > 
> > *((struct efi_device_path *)buf) = END;
> > 
> 
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/2] efi_loader: initialize ethernet device path fully

[Heinrich Schuchardt]

On 03/23/2018 07:54 PM, Heinrich Schuchardt wrote:
>>From c38a011f699cf5a41b48c6fc547f3d0dde3d7cf9 Mon Sep 17 00:00:00 2001
> From: Patrick Wildt 
> Date: Fri, 23 Mar 2018 15:36:58 +0100
> Subject: [PATCH 1/2] efi_loader: initialize ethernet device path fully
> 
> Since the backing memory for a new device path can contain stale
> data we have to make sure that we either zero the buffer or that
> we initialize all variables in the overlaying structure.  This
> fixes setting the boot device path in the loaded image protocol
> in case we boot from network.>
> Signed-off-by: Patrick Wildt 
> ---
>  lib/efi_loader/efi_device_path.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/efi_loader/efi_device_path.c
> b/lib/efi_loader/efi_device_path.c
> index 3c735e60d3..a086c7bbd4 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -777,7 +777,9 @@ struct efi_device_path *efi_dp_from_eth(void)
>   ndp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
>   ndp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_MAC_ADDR;
>   ndp->dp.length = sizeof(*ndp);
> - memcpy(ndp->mac.addr, eth_get_ethaddr(), ARP_HLEN);
> + memset(>mac, 0, sizeof(ndp->mac));
> + memcpy(>mac, eth_get_ethaddr(), ARP_HLEN);

I would prefer if you would set the memory to zero in dp_alloc(). This
would avoid the observed problem in all device paths.

> + ndp->if_type = 1;

This line does not give any clue what 1 relates to. Please, define a
constant. EDK uses NET_IFTYPE_ETHERNET = 1.

The UEFI spec refers to RFC3232. But I could not find the corresponding
definition there.
https://www.iana.org/assignments/ianaiftype-mib/ianaiftype-mib uses the
value 6 for Ethernet interfaces.

Please, provide the RFC or IANA page from which you have taken the value
1 in a comment for the constant.

Best regards

Heinrich

>   buf = [1];
> 
>   *((struct efi_device_path *)buf) = END;
> 

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot