Re: [U-Boot] [PATCH 1/2] efi_loader: initialize ethernet device path fully
On 03/23/2018 07:54 PM, Heinrich Schuchardt wrote:
From c38a011f699cf5a41b48c6fc547f3d0dde3d7cf9 Mon Sep 17 00:00:00 2001
From: Patrick Wildt patrick@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@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.
- 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 = &ndp[1];
*((struct efi_device_path *)buf) = END;
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@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@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.
- 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 = &ndp[1];
*((struct efi_device_path *)buf) = END;
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@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@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;
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 patrick@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@blueri.se
<snip>
- 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
participants (2)
-
Heinrich Schuchardt -
Patrick Wildt