efi_loader: initialize ethernet device path fully

PATCH New Heinrich Schuchardt 2 participants 4 messages Mar 23, 2018 – Mar 26, 2018 1 message not in the archive

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Heinrich Schuchardt

author

2018-03-23 18:58 UTC #1 lore raw

PATCH 1/2 efi_loader: initialize ethernet device path fully

Quoted 21 lines · Heinrich Schuchardt

Quoted 1 line
From c38a011f699cf5a41b48c6fc547f3d0dde3d7cf9 Mon Sep 17 00:00:00 2001
From: Patrick Wildt <[email protected]>
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 <[email protected]>
---
 lib/efi_loader/efi_device_path.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
lib/efi_loader/efi_device_path.c +0 -0
index 3c735e60d3..a086c7bbd4 100644

lib/efi_loader/efi_device_path.c struct efi_device_path *efi_dp_from_eth(void) · line 781

Quoted 6 lines

lib/efi_loader/efi_device_path.c +2 -1

@@ -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.

lib/efi_loader/efi_device_path.c struct efi_device_path *efi_dp_from_eth(void) · line 782

Quoted 1 line

lib/efi_loader/efi_device_path.c +1 -0
+	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

Quoted 4 lines

lib/efi_loader/efi_device_path.c +0 -0

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

Patrick Wildt

2018-03-25 17:31 UTC #2 lore raw

PATCH 1/2 efi_loader: initialize ethernet device path fully

Quoted 16 lines · Heinrich Schuchardt

Quoted 15 lines · Heinrich Schuchardt
Quoted 1 line
From c38a011f699cf5a41b48c6fc547f3d0dde3d7cf9 Mon Sep 17 00:00:00 2001
From: Patrick Wildt <[email protected]>
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 <[email protected]>
---
 lib/efi_loader/efi_device_path.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Quoted 15 lines

lib/efi_loader/efi_device_path.c struct efi_device_path *efi_dp_from_eth(void) · line 781
Quoted 12 lines
lib/efi_loader/efi_device_path.c +2 -1
index 3c735e60d3..a086c7bbd4 100644
@@ -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.

Quoted 6 lines

lib/efi_loader/efi_device_path.c struct efi_device_path *efi_dp_from_eth(void) · line 782
Quoted 2 lines
lib/efi_loader/efi_device_path.c +1 -0
+	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.

Quoted 7 lines

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

Quoted 9 lines

Best regards
Heinrich
Quoted 4 lines
lib/efi_loader/efi_device_path.c +0 -0
 	buf = &ndp[1];
 
 	*((struct efi_device_path *)buf) = END;

Heinrich Schuchardt

author

2018-03-25 20:11 UTC #3 lore raw

PATCH 1/2 efi_loader: initialize ethernet device path fully

Quoted 33 lines · Patrick Wildt

Quoted 16 lines · Heinrich Schuchardt
Quoted 15 lines · Heinrich Schuchardt
Quoted 1 line
From c38a011f699cf5a41b48c6fc547f3d0dde3d7cf9 Mon Sep 17 00:00:00 2001
From: Patrick Wildt <[email protected]>
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 <[email protected]>
---
 lib/efi_loader/efi_device_path.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Quoted 15 lines
lib/efi_loader/efi_device_path.c struct efi_device_path *efi_dp_from_eth(void) · line 781
Quoted 12 lines
lib/efi_loader/efi_device_path.c +2 -1
index 3c735e60d3..a086c7bbd4 100644
@@ -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.

Quoted 2 lines

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

I already reviewed that one. Thanks.

Quoted 19 lines

Quoted 7 lines
lib/efi_loader/efi_device_path.c struct efi_device_path *efi_dp_from_eth(void) · line 782
Quoted 3 lines
lib/efi_loader/efi_device_path.c +1 -0
+	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.
Quoted 7 lines
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 :

Review

Quoted 2 lines

(quoted diff) +2 -0

+		/* 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

Quoted 17 lines

I don't know if setting this even matters, I added it for consistency, but left out the comment.
Best regards,
Patrick
Quoted 9 lines
Best regards
Heinrich
Quoted 4 lines
lib/efi_loader/efi_device_path.c +0 -0
 	buf = &ndp[1];
 
 	*((struct efi_device_path *)buf) = END;

Heinrich Schuchardt

author

2018-03-26 05:13 UTC #4 lore raw

PATCH 1/2 efi_loader: initialize ethernet device path fully

Quoted 15 lines · Heinrich Schuchardt

Quoted 14 lines · Patrick Wildt
Quoted 13 lines · Heinrich Schuchardt
Quoted 12 lines · Heinrich Schuchardt
Quoted 1 line
From c38a011f699cf5a41b48c6fc547f3d0dde3d7cf9 Mon Sep 17 00:00:00 2001
From: Patrick Wildt <[email protected]>
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 <[email protected]>
---

<snip>

Quoted 26 lines

Quoted 20 lines
Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +1 -0
+	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.
Quoted 7 lines
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 :
Review
Quoted 2 lines
(quoted diff) +2 -0
+		/* 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.

Quoted 4 lines

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