Re: [PATCH net-next v2 8/8] net: core: Convert dev_set_mac_address_user() to use struct sockaddr_storage

["Gustavo A. R. Silva" <gustavo@embeddedor.com>]

On 21/05/25 14:46, Kees Cook wrote:
> Convert callers of dev_set_mac_address_user() to use struct
> sockaddr_storage. Add sanity checks on dev->addr_len usage.
> 
> Signed-off-by: Kees Cook <kees@kernel.org>

Acked-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Thanks!
-Gustavo

> ---
> Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Andrew Lunn <andrew+netdev@lunn.ch>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Simon Horman <horms@kernel.org>
> Cc: Stanislav Fomichev <sdf@fomichev.me>
> Cc: Cosmin Ratiu <cratiu@nvidia.com>
> Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
> Cc: Florian Fainelli <florian.fainelli@broadcom.com>
> Cc: Kory Maincent <kory.maincent@bootlin.com>
> Cc: Maxim Georgiev <glipus@gmail.com>
> Cc: Kuniyuki Iwashima <kuniyu@amazon.com>
> Cc: <netdev@vger.kernel.org>
> ---
>   include/linux/netdevice.h |  2 +-
>   drivers/net/tap.c         | 14 +++++++++-----
>   drivers/net/tun.c         |  8 +++++++-
>   net/core/dev_api.c        |  5 +++--
>   net/core/dev_ioctl.c      |  6 ++++--
>   5 files changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index b4242b997373..adb14db25798 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -4216,7 +4216,7 @@ int netif_set_mac_address(struct net_device *dev, struct sockaddr_storage *ss,
>   			  struct netlink_ext_ack *extack);
>   int dev_set_mac_address(struct net_device *dev, struct sockaddr_storage *ss,
>   			struct netlink_ext_ack *extack);
> -int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa,
> +int dev_set_mac_address_user(struct net_device *dev, struct sockaddr_storage *ss,
>   			     struct netlink_ext_ack *extack);
>   int dev_get_mac_address(struct sockaddr *sa, struct net *net, char *dev_name);
>   int dev_get_port_parent_id(struct net_device *dev,
> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> index d4ece538f1b2..bdf0788d8e66 100644
> --- a/drivers/net/tap.c
> +++ b/drivers/net/tap.c
> @@ -923,7 +923,7 @@ static long tap_ioctl(struct file *file, unsigned int cmd,
>   	unsigned int __user *up = argp;
>   	unsigned short u;
>   	int __user *sp = argp;
> -	struct sockaddr sa;
> +	struct sockaddr_storage ss;
>   	int s;
>   	int ret;
>   
> @@ -1000,16 +1000,17 @@ static long tap_ioctl(struct file *file, unsigned int cmd,
>   			return -ENOLINK;
>   		}
>   		ret = 0;
> -		dev_get_mac_address(&sa, dev_net(tap->dev), tap->dev->name);
> +		dev_get_mac_address((struct sockaddr *)&ss, dev_net(tap->dev),
> +				    tap->dev->name);
>   		if (copy_to_user(&ifr->ifr_name, tap->dev->name, IFNAMSIZ) ||
> -		    copy_to_user(&ifr->ifr_hwaddr, &sa, sizeof(sa)))
> +		    copy_to_user(&ifr->ifr_hwaddr, &ss, sizeof(ifr->ifr_hwaddr)))
>   			ret = -EFAULT;
>   		tap_put_tap_dev(tap);
>   		rtnl_unlock();
>   		return ret;
>   
>   	case SIOCSIFHWADDR:
> -		if (copy_from_user(&sa, &ifr->ifr_hwaddr, sizeof(sa)))
> +		if (copy_from_user(&ss, &ifr->ifr_hwaddr, sizeof(ifr->ifr_hwaddr)))
>   			return -EFAULT;
>   		rtnl_lock();
>   		tap = tap_get_tap_dev(q);
> @@ -1017,7 +1018,10 @@ static long tap_ioctl(struct file *file, unsigned int cmd,
>   			rtnl_unlock();
>   			return -ENOLINK;
>   		}
> -		ret = dev_set_mac_address_user(tap->dev, &sa, NULL);
> +		if (tap->dev->addr_len > sizeof(ifr->ifr_hwaddr))
> +			ret = -EINVAL;
> +		else
> +			ret = dev_set_mac_address_user(tap->dev, &ss, NULL);
>   		tap_put_tap_dev(tap);
>   		rtnl_unlock();
>   		return ret;
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 7babd1e9a378..1207196cbbed 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -3193,7 +3193,13 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
>   
>   	case SIOCSIFHWADDR:
>   		/* Set hw address */
> -		ret = dev_set_mac_address_user(tun->dev, &ifr.ifr_hwaddr, NULL);
> +		if (tun->dev->addr_len > sizeof(ifr.ifr_hwaddr)) {
> +			ret = -EINVAL;
> +			break;
> +		}
> +		ret = dev_set_mac_address_user(tun->dev,
> +					       (struct sockaddr_storage *)&ifr.ifr_hwaddr,
> +					       NULL);
>   		break;
>   
>   	case TUNGETSNDBUF:
> diff --git a/net/core/dev_api.c b/net/core/dev_api.c
> index 6011a5ef649d..1bf0153195f2 100644
> --- a/net/core/dev_api.c
> +++ b/net/core/dev_api.c
> @@ -84,14 +84,15 @@ void dev_set_group(struct net_device *dev, int new_group)
>   	netdev_unlock_ops(dev);
>   }
>   
> -int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa,
> +int dev_set_mac_address_user(struct net_device *dev,
> +			     struct sockaddr_storage *ss,
>   			     struct netlink_ext_ack *extack)
>   {
>   	int ret;
>   
>   	down_write(&dev_addr_sem);
>   	netdev_lock_ops(dev);
> -	ret = netif_set_mac_address(dev, (struct sockaddr_storage *)sa, extack);
> +	ret = netif_set_mac_address(dev, ss, extack);
>   	netdev_unlock_ops(dev);
>   	up_write(&dev_addr_sem);
>   
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index fff13a8b48f1..616479e71466 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -572,9 +572,11 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
>   		return dev_set_mtu(dev, ifr->ifr_mtu);
>   
>   	case SIOCSIFHWADDR:
> -		if (dev->addr_len > sizeof(struct sockaddr))
> +		if (dev->addr_len > sizeof(ifr->ifr_hwaddr))
>   			return -EINVAL;
> -		return dev_set_mac_address_user(dev, &ifr->ifr_hwaddr, NULL);
> +		return dev_set_mac_address_user(dev,
> +						(struct sockaddr_storage *)&ifr->ifr_hwaddr,
> +						NULL);
>   
>   	case SIOCSIFHWBROADCAST:
>   		if (ifr->ifr_hwaddr.sa_family != dev->type)