* [Patch net] rtnetlink: allocate more memory for dev_set_mac_address() @ 2017-07-20 18:27 Cong Wang 2017-07-20 18:27 ` [Patch net] net: check mac address length " Cong Wang 2017-07-20 22:23 ` [Patch net] rtnetlink: allocate more memory " David Miller 0 siblings, 2 replies; 5+ messages in thread From: Cong Wang @ 2017-07-20 18:27 UTC (permalink / raw) To: netdev; +Cc: dsahern, Cong Wang virtnet_set_mac_address() interprets mac address as struct sockaddr, but upper layer only allocates dev->addr_len which is ETH_ALEN + sizeof(sa_family_t) in this case. We lack a unified definition for mac address, so just fix the upper layer, this also allows drivers to interpret it to struct sockaddr freely. Reported-by: David Ahern <dsahern@gmail.com> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- net/core/rtnetlink.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index d1ba90980be1..ebaa26cf777f 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -2031,7 +2031,8 @@ static int do_setlink(const struct sk_buff *skb, struct sockaddr *sa; int len; - len = sizeof(sa_family_t) + dev->addr_len; + len = sizeof(sa_family_t) + max_t(size_t, dev->addr_len, + sizeof(*sa)); sa = kmalloc(len, GFP_KERNEL); if (!sa) { err = -ENOMEM; -- 2.13.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [Patch net] net: check mac address length for dev_set_mac_address() 2017-07-20 18:27 [Patch net] rtnetlink: allocate more memory for dev_set_mac_address() Cong Wang @ 2017-07-20 18:27 ` Cong Wang 2017-07-20 23:39 ` Cong Wang 2017-07-21 18:50 ` Cong Wang 2017-07-20 22:23 ` [Patch net] rtnetlink: allocate more memory " David Miller 1 sibling, 2 replies; 5+ messages in thread From: Cong Wang @ 2017-07-20 18:27 UTC (permalink / raw) To: netdev; +Cc: dsahern, Cong Wang, Jiri Pirko dev_set_mac_address() accepts a struct sockaddr pointer as input but we have various types of mac addresse whose lengths are up to MAX_ADDR_LEN, this is confusing. Make it void like ->ndo_set_mac_address() and let callers check its length before calling it. It is too late to fix dev_ifsioc() due to API compatibility, so just reject those larger than sizeof(struct sockaddr). Fortunately, only a few IPv6 tunnel devices have addr_len larger than sizeof(struct sockaddr) and they don't support ndo_set_mac_addr(). But team driver seems still buggy without this patch. Cc: Jiri Pirko <jiri@resnulli.us> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- include/linux/netdevice.h | 2 +- net/core/dev.c | 10 +++++++--- net/core/dev_ioctl.c | 2 ++ 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 779b23595596..d7e872fa4656 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -3287,7 +3287,7 @@ int dev_change_net_namespace(struct net_device *, struct net *, const char *); int __dev_set_mtu(struct net_device *, int); int dev_set_mtu(struct net_device *, int); void dev_set_group(struct net_device *, int); -int dev_set_mac_address(struct net_device *, struct sockaddr *); +int dev_set_mac_address(struct net_device *, void *); int dev_change_carrier(struct net_device *, bool new_carrier); int dev_get_phys_port_id(struct net_device *dev, struct netdev_phys_item_id *ppid); diff --git a/net/core/dev.c b/net/core/dev.c index 02440518dd69..1802303bd0a7 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -6844,13 +6844,17 @@ EXPORT_SYMBOL(dev_set_group); /** * dev_set_mac_address - Change Media Access Control Address * @dev: device - * @sa: new address + * @addr: new address, whose type could be either struct sockaddr or + * any other compatible type whose length is up to MAX_ADDR_LEN depending + * on the dev->addr_len. Callers should check if its length is smaller than + * dev->addr_len!! * * Change the hardware (MAC) address of the device */ -int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa) +int dev_set_mac_address(struct net_device *dev, void *addr) { const struct net_device_ops *ops = dev->netdev_ops; + struct sockaddr *sa = addr; int err; if (!ops->ndo_set_mac_address) @@ -6859,7 +6863,7 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa) return -EINVAL; if (!netif_device_present(dev)) return -ENODEV; - err = ops->ndo_set_mac_address(dev, sa); + err = ops->ndo_set_mac_address(dev, addr); if (err) return err; dev->addr_assign_type = NET_ADDR_SET; diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c index 82fd4c9c4a1b..3f41601d7b7c 100644 --- a/net/core/dev_ioctl.c +++ b/net/core/dev_ioctl.c @@ -262,6 +262,8 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd) return dev_set_mtu(dev, ifr->ifr_mtu); case SIOCSIFHWADDR: + if (dev->addr_len > sizeof(struct sockaddr)) + return -EINVAL; return dev_set_mac_address(dev, &ifr->ifr_hwaddr); case SIOCSIFHWBROADCAST: -- 2.13.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Patch net] net: check mac address length for dev_set_mac_address() 2017-07-20 18:27 ` [Patch net] net: check mac address length " Cong Wang @ 2017-07-20 23:39 ` Cong Wang 2017-07-21 18:50 ` Cong Wang 1 sibling, 0 replies; 5+ messages in thread From: Cong Wang @ 2017-07-20 23:39 UTC (permalink / raw) To: Linux Kernel Network Developers; +Cc: David Ahern, Cong Wang, Jiri Pirko On Thu, Jul 20, 2017 at 11:27 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote: > dev_set_mac_address() accepts a struct sockaddr pointer as > input but we have various types of mac addresse whose lengths > are up to MAX_ADDR_LEN, this is confusing. > > Make it void like ->ndo_set_mac_address() and let callers check > its length before calling it. It is too late to fix dev_ifsioc() > due to API compatibility, so just reject those larger than > sizeof(struct sockaddr). > > Fortunately, only a few IPv6 tunnel devices have addr_len > larger than sizeof(struct sockaddr) and they don't support > ndo_set_mac_addr(). But team driver seems still buggy without > this patch. Note, in team lb mode, I can successfully enslave ip6gre device to a team device and make its mac addr look like: # ip li show dev team0 17: team0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1448 qdisc noqueue state UP mode DEFAULT group default qlen 1000 link/gre6 00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00 brd 00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00 But ifconfig seems not recognize gre6 family, so it is just a matter of a few lines of C code to trigger the bug. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Patch net] net: check mac address length for dev_set_mac_address() 2017-07-20 18:27 ` [Patch net] net: check mac address length " Cong Wang 2017-07-20 23:39 ` Cong Wang @ 2017-07-21 18:50 ` Cong Wang 1 sibling, 0 replies; 5+ messages in thread From: Cong Wang @ 2017-07-21 18:50 UTC (permalink / raw) To: Linux Kernel Network Developers; +Cc: David Ahern, Jiri Pirko On Thu, Jul 20, 2017 at 11:27 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote: > diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c > index 82fd4c9c4a1b..3f41601d7b7c 100644 > --- a/net/core/dev_ioctl.c > +++ b/net/core/dev_ioctl.c > @@ -262,6 +262,8 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd) > return dev_set_mtu(dev, ifr->ifr_mtu); > > case SIOCSIFHWADDR: > + if (dev->addr_len > sizeof(struct sockaddr)) > + return -EINVAL; > return dev_set_mac_address(dev, &ifr->ifr_hwaddr); > Thinking a bit more, I should keep this patch simpler for -net and -stable, so only the above piece is necessary, the rest pieces can be put to -net-next. I will resend this, and the "team: use a larger struct for mac address". Thanks. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Patch net] rtnetlink: allocate more memory for dev_set_mac_address() 2017-07-20 18:27 [Patch net] rtnetlink: allocate more memory for dev_set_mac_address() Cong Wang 2017-07-20 18:27 ` [Patch net] net: check mac address length " Cong Wang @ 2017-07-20 22:23 ` David Miller 1 sibling, 0 replies; 5+ messages in thread From: David Miller @ 2017-07-20 22:23 UTC (permalink / raw) To: xiyou.wangcong; +Cc: netdev, dsahern From: Cong Wang <xiyou.wangcong@gmail.com> Date: Thu, 20 Jul 2017 11:27:57 -0700 > virtnet_set_mac_address() interprets mac address as struct > sockaddr, but upper layer only allocates dev->addr_len > which is ETH_ALEN + sizeof(sa_family_t) in this case. > > We lack a unified definition for mac address, so just fix > the upper layer, this also allows drivers to interpret it > to struct sockaddr freely. > > Reported-by: David Ahern <dsahern@gmail.com> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> Applied and queued up for -stable, thanks Cong. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-07-21 18:50 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-07-20 18:27 [Patch net] rtnetlink: allocate more memory for dev_set_mac_address() Cong Wang 2017-07-20 18:27 ` [Patch net] net: check mac address length " Cong Wang 2017-07-20 23:39 ` Cong Wang 2017-07-21 18:50 ` Cong Wang 2017-07-20 22:23 ` [Patch net] rtnetlink: allocate more memory " David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).