linux-kernel - Re: [PATCHv2 net] usbnet: modern method to get random MAC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89i+m69mWQw+V6XWCzmF84s6uQV15m_YdkPDQptoxUks4=w@mail.gmail.com>
Date: Mon, 14 Oct 2024 21:59:33 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Oliver Neukum <oneukum@...e.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, 
	netdev@...r.kernel.org, linux-usb@...r.kernel.org, 
	linux-kernel@...r.kernel.org, stable@...nel.org, 
	John Sperbeck <jsperbeck@...gle.com>, Brian Vazquez <brianvv@...gle.com>
Subject: Re: [PATCHv2 net] usbnet: modern method to get random MAC

On Thu, Aug 29, 2024 at 7:52 PM Oliver Neukum <oneukum@...e.com> wrote:
>
> The driver generates a random MAC once on load
> and uses it over and over, including on two devices
> needing a random MAC at the same time.
>
> Jakub suggested revamping the driver to the modern
> API for setting a random MAC rather than fixing
> the old stuff.
>
> The bug is as old as the driver.
>
> Signed-off-by: Oliver Neukum <oneukum@...e.com>
>
> ---
>
> v2: Correct commentary style
>
>  drivers/net/usb/usbnet.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index dfc37016690b..40536e1cb4df 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -61,9 +61,6 @@
>
>  /*-------------------------------------------------------------------------*/
>
> -// randomly generated ethernet address
> -static u8      node_id [ETH_ALEN];
> -
>  /* use ethtool to change the level for any given device */
>  static int msg_level = -1;
>  module_param (msg_level, int, 0);
> @@ -1743,7 +1740,6 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
>
>         dev->net = net;
>         strscpy(net->name, "usb%d", sizeof(net->name));
> -       eth_hw_addr_set(net, node_id);
>
>         /* rx and tx sides can use different message sizes;
>          * bind() should set rx_urb_size in that case.
> @@ -1819,9 +1815,9 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
>                 goto out4;
>         }
>
> -       /* let userspace know we have a random address */
> -       if (ether_addr_equal(net->dev_addr, node_id))
> -               net->addr_assign_type = NET_ADDR_RANDOM;
> +       /* this flags the device for user space */
> +       if (!is_valid_ether_addr(net->dev_addr))
> +               eth_hw_addr_random(net);
>
>         if ((dev->driver_info->flags & FLAG_WLAN) != 0)
>                 SET_NETDEV_DEVTYPE(net, &wlan_type);
> @@ -2229,7 +2225,6 @@ static int __init usbnet_init(void)
>         BUILD_BUG_ON(
>                 sizeof_field(struct sk_buff, cb) < sizeof(struct skb_data));
>
> -       eth_random_addr(node_id);
>         return 0;
>  }
>  module_init(usbnet_init);
> --
> 2.45.2
>

As diagnosed by John Sperbeck :

This patch implies all ->bind() method took care of populating net->dev_addr ?

Otherwise the following existing heuristic is no longer working

// heuristic:  "usb%d" for links we know are two-host,
// else "eth%d" when there's reasonable doubt.  userspace
// can rename the link if it knows better.
if ((dev->driver_info->flags & FLAG_ETHER) != 0 &&
    ((dev->driver_info->flags & FLAG_POINTTOPOINT) == 0 ||
     (net->dev_addr [0] & 0x02) == 0))
strscpy(net->name, "eth%d", sizeof(net->name));