usb: gadget: u_ether: fix race in setting MAC address in setup phase

PATCH Superseded Marian Postevca 2 participants 4 messages Nov 29, 2021 – Dec 4, 2021 USB

Share & tools

Marian Postevca

author

2021-11-29 22:30 UTC #0 lore raw

PATCH 1/1 usb: gadget: u_ether: fix race in setting MAC address in setup phase

When listening for notifications through netlink of a new interface being registered, sporadically, it is possible for the MAC to be read as zero. The zero MAC address lasts a short period of time and then switches to a valid random MAC address.

This causes problems for netd in Android, which assumes that the interface is malfunctioning and will not use it.

In the good case we get this log:
InterfaceController::getCfg() ifName usb0
hwAddr 92:a8:f0:73:79:5b ipv4Addr 0.0.0.0 flags 0x1002

In the error case we get these logs:
InterfaceController::getCfg() ifName usb0
hwAddr 00:00:00:00:00:00 ipv4Addr 0.0.0.0 flags 0x1002

netd : interfaceGetCfg("usb0")
netd : interfaceSetCfg() -> ServiceSpecificException
(99, "[Cannot assign requested address] : ioctl() failed")

The reason for the issue is the order in which the interface is setup, it is first registered through register_netdev() and after the MAC address is set.

Fixed by first setting the MAC address of the net_device and after that calling register_netdev().

Signed-off-by Marian Postevca <[email protected]>

---
 drivers/usb/gadget/function/u_ether.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

drivers/usb/gadget/function/u_ether.c +6 ~~-10~~

index e0ad5aed6ac9..6f5d45ef2e39 100644
@@ -17,6 +17,7 @@
 #include <linux/etherdevice.h>
 #include <linux/ethtool.h>
 #include <linux/if_vlan.h>
+#include <linux/etherdevice.h>
 
 #include "u_ether.h"

@@ -863,19 +864,23 @@ int gether_register_netdev(struct net_device *net)
 {
 	struct eth_dev *dev;
 	struct usb_gadget *g;
-	struct sockaddr sa;
 	int status;
 
 	if (!net->dev.parent)
 		return -EINVAL;
 	dev = netdev_priv(net);
 	g = dev->gadget;
+
+	net->addr_assign_type = NET_ADDR_RANDOM;
+	eth_hw_addr_set(net, dev->dev_mac);
+
 	status = register_netdev(net);
 	if (status < 0) {
 		dev_dbg(&g->dev, "register_netdev failed, %d\n", status);
 		return status;
 	} else {
 		INFO(dev, "HOST MAC %pM\n", dev->host_mac);
+		INFO(dev, "MAC %pM\n", dev->dev_mac);
 
 		/* two kinds of host-initiated state changes:
 		 *  - iff DATA transfer is active, carrier is "on"

@@ -883,15 +888,6 @@ int gether_register_netdev(struct net_device *net)
 		 */
 		netif_carrier_off(net);
 	}
-	sa.sa_family = net->type;
-	memcpy(sa.sa_data, dev->dev_mac, ETH_ALEN);
-	rtnl_lock();
-	status = dev_set_mac_address(net, &sa, NULL);
-	rtnl_unlock();
-	if (status)
-		pr_warn("cannot set self ethernet address: %d\n", status);
-	else
-		INFO(dev, "MAC %pM\n", dev->dev_mac);
 
 	return status;
 }

Signature · 1 line

2.32.0

Greg Kroah-Hartman

maintainer

2021-12-03 12:50 UTC #1 lore raw

Quoted 24 lines · Marian Postevca

When listening for notifications through netlink of a new interface being registered, sporadically, it is possible for the MAC to be read as zero. The zero MAC address lasts a short period of time and then switches to a valid random MAC address.
This causes problems for netd in Android, which assumes that the interface is malfunctioning and will not use it.
In the good case we get this log:
InterfaceController::getCfg() ifName usb0
hwAddr 92:a8:f0:73:79:5b ipv4Addr 0.0.0.0 flags 0x1002
In the error case we get these logs:
InterfaceController::getCfg() ifName usb0
hwAddr 00:00:00:00:00:00 ipv4Addr 0.0.0.0 flags 0x1002
netd : interfaceGetCfg("usb0")
netd : interfaceSetCfg() -> ServiceSpecificException
(99, "[Cannot assign requested address] : ioctl() failed")
The reason for the issue is the order in which the interface is setup, it is first registered through register_netdev() and after the MAC address is set.

Quoted 7 lines

Fixed by first setting the MAC address of the net_device and after that calling register_netdev().
Signed-off-by Marian Postevca <[email protected]>
---
 drivers/usb/gadget/function/u_ether.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

What commit does this fix? Should it go to stable kernel releases?

thanks,

greg k-h

Marian Postevca

author

2021-12-04 19:00 UTC #2 lore raw

Quoted 33 lines · Greg Kroah-Hartman

Quoted 24 lines · Marian Postevca
When listening for notifications through netlink of a new interface being registered, sporadically, it is possible for the MAC to be read as zero. The zero MAC address lasts a short period of time and then switches to a valid random MAC address.
This causes problems for netd in Android, which assumes that the interface is malfunctioning and will not use it.
In the good case we get this log:
InterfaceController::getCfg() ifName usb0
hwAddr 92:a8:f0:73:79:5b ipv4Addr 0.0.0.0 flags 0x1002
In the error case we get these logs:
InterfaceController::getCfg() ifName usb0
hwAddr 00:00:00:00:00:00 ipv4Addr 0.0.0.0 flags 0x1002
netd : interfaceGetCfg("usb0")
netd : interfaceSetCfg() -> ServiceSpecificException
(99, "[Cannot assign requested address] : ioctl() failed")
The reason for the issue is the order in which the interface is setup, it is first registered through register_netdev() and after the MAC address is set.
Quoted 7 lines
Fixed by first setting the MAC address of the net_device and after that calling register_netdev().
Signed-off-by Marian Postevca <[email protected]>
---
 drivers/usb/gadget/function/u_ether.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

Quoted 5 lines

What commit does this fix? Should it go to stable kernel releases?
thanks,
greg k-h

This fixes bcd4a1c40bee885e ("usb: gadget: u_ether: construct with default values and add setters/getters").

I think it should go to stable kernel releases.

Should I send a second version of the patch with a Fixes tag?

Thanks

Greg Kroah-Hartman

maintainer

2021-12-04 18:47 UTC #3 lore raw

Quoted 39 lines · Marian Postevca

Quoted 37 lines · Greg Kroah-Hartman
Quoted 24 lines · Marian Postevca
When listening for notifications through netlink of a new interface being registered, sporadically, it is possible for the MAC to be read as zero. The zero MAC address lasts a short period of time and then switches to a valid random MAC address.
This causes problems for netd in Android, which assumes that the interface is malfunctioning and will not use it.
In the good case we get this log:
InterfaceController::getCfg() ifName usb0
hwAddr 92:a8:f0:73:79:5b ipv4Addr 0.0.0.0 flags 0x1002
In the error case we get these logs:
InterfaceController::getCfg() ifName usb0
hwAddr 00:00:00:00:00:00 ipv4Addr 0.0.0.0 flags 0x1002
netd : interfaceGetCfg("usb0")
netd : interfaceSetCfg() -> ServiceSpecificException
(99, "[Cannot assign requested address] : ioctl() failed")
The reason for the issue is the order in which the interface is setup, it is first registered through register_netdev() and after the MAC address is set.
Quoted 7 lines
Fixed by first setting the MAC address of the net_device and after that calling register_netdev().
Signed-off-by Marian Postevca <[email protected]>
---
 drivers/usb/gadget/function/u_ether.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)
What commit does this fix? Should it go to stable kernel releases?
thanks,

Quoted 8 lines

Quoted 1 line
greg k-h
This fixes bcd4a1c40bee885e ("usb: gadget: u_ether: construct with default values and add setters/getters").
I think it should go to stable kernel releases.
Should I send a second version of the patch with a Fixes tag?

Yes please, and a cc: stable on the line after the fixes tag.

thanks,

greg k-h