[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 <patrick at blueri.se>
>>> 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 <patrick at blueri.se>
>>> ---
>>>  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(&ndp->mac, 0, sizeof(ndp->mac));
>>> +	memcpy(&ndp->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 = &ndp[1];
>>>
>>>  	*((struct efi_device_path *)buf) = END;
>>>
>>
>