| Message ID | 20241217012445.work.979-kees@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: core: dev.c confirmed to use classic sockaddr | expand |
Kees Cook wrote: > As part of trying to clean up struct sock_addr, add comments about the > sockaddr arguments of dev_[gs]et_mac_address() being actual classic "max > 14 bytes in sa_data" sockaddr instances and not struct sockaddr_storage. What is this assertion based on? I see various non-Ethernet .ndo_set_mac_address implementations, which dev_set_mac_address calls. And dev_set_mac_addr_user is called from rtnetlink do_setlink. Which kmalloc's sa based on dev->addr_len. > > Signed-off-by: Kees Cook <kees@kernel.org> > --- > 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: netdev@vger.kernel.org > --- > net/core/dev.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index 45a8c3dd4a64..5abfd29a35bf 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -9183,7 +9183,8 @@ EXPORT_SYMBOL(dev_pre_changeaddr_notify); > /** > * dev_set_mac_address - Change Media Access Control Address > * @dev: device > - * @sa: new address > + * @sa: new address in a classic "struct sockaddr", which will never > + * have more than 14 bytes in @sa::sa_data > * @extack: netlink extended ack > * > * Change the hardware (MAC) address of the device > @@ -9217,6 +9218,7 @@ EXPORT_SYMBOL(dev_set_mac_address); > > DECLARE_RWSEM(dev_addr_sem); > > +/* "sa" is a classic sockaddr: it will only ever use 14 bytes from sa_data. */ > int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa, > struct netlink_ext_ack *extack) > { > @@ -9229,6 +9231,7 @@ int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa, > } > EXPORT_SYMBOL(dev_set_mac_address_user); > > +/* "sa" is a classic sockaddr: it will only ever use 14 bytes from sa_data. */ > int dev_get_mac_address(struct sockaddr *sa, struct net *net, char *dev_name) > { > size_t size = sizeof(sa->sa_data_min); > -- > 2.34.1 >
On Tue, Dec 17, 2024 at 10:35:44AM -0500, Willem de Bruijn wrote: > Kees Cook wrote: > > As part of trying to clean up struct sock_addr, add comments about the > > sockaddr arguments of dev_[gs]et_mac_address() being actual classic "max > > 14 bytes in sa_data" sockaddr instances and not struct sockaddr_storage. > > What is this assertion based on? > > I see various non-Ethernet .ndo_set_mac_address implementations, which > dev_set_mac_address calls. And dev_set_mac_addr_user is called from > rtnetlink do_setlink. Which kmalloc's sa based on dev->addr_len. Yeah, I was clearly missing several cases. Please ignore this patch. I will re-examine this.
On Tue, Dec 17, 2024 at 12:30:36PM -0800, Kees Cook wrote: > On Tue, Dec 17, 2024 at 10:35:44AM -0500, Willem de Bruijn wrote: > > Kees Cook wrote: > > > As part of trying to clean up struct sock_addr, add comments about the > > > sockaddr arguments of dev_[gs]et_mac_address() being actual classic "max > > > 14 bytes in sa_data" sockaddr instances and not struct sockaddr_storage. > > > > What is this assertion based on? > > > > I see various non-Ethernet .ndo_set_mac_address implementations, which > > dev_set_mac_address calls. And dev_set_mac_addr_user is called from > > rtnetlink do_setlink. Which kmalloc's sa based on dev->addr_len. > > Yeah, I was clearly missing several cases. Please ignore this patch. I > will re-examine this. So, I think I see what happened -- I missed the dev->addr_len in dev_set_mac_address(), and saw that dev_get_mac_address() caps the address to 14: size_t size = sizeof(sa->sa_data_min); ... if (!dev->addr_len) memset(sa->sa_data, 0, size); else memcpy(sa->sa_data, dev->dev_addr, min_t(size_t, size, dev->addr_len)); It seems only tun/tap and SIOCGIFHWADDR use the "get" interface, though.
diff --git a/net/core/dev.c b/net/core/dev.c index 45a8c3dd4a64..5abfd29a35bf 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -9183,7 +9183,8 @@ EXPORT_SYMBOL(dev_pre_changeaddr_notify); /** * dev_set_mac_address - Change Media Access Control Address * @dev: device - * @sa: new address + * @sa: new address in a classic "struct sockaddr", which will never + * have more than 14 bytes in @sa::sa_data * @extack: netlink extended ack * * Change the hardware (MAC) address of the device @@ -9217,6 +9218,7 @@ EXPORT_SYMBOL(dev_set_mac_address); DECLARE_RWSEM(dev_addr_sem); +/* "sa" is a classic sockaddr: it will only ever use 14 bytes from sa_data. */ int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa, struct netlink_ext_ack *extack) { @@ -9229,6 +9231,7 @@ int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa, } EXPORT_SYMBOL(dev_set_mac_address_user); +/* "sa" is a classic sockaddr: it will only ever use 14 bytes from sa_data. */ int dev_get_mac_address(struct sockaddr *sa, struct net *net, char *dev_name) { size_t size = sizeof(sa->sa_data_min);
As part of trying to clean up struct sock_addr, add comments about the sockaddr arguments of dev_[gs]et_mac_address() being actual classic "max 14 bytes in sa_data" sockaddr instances and not struct sockaddr_storage. Signed-off-by: Kees Cook <kees@kernel.org> --- 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: netdev@vger.kernel.org --- net/core/dev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)