| Message ID | 20241217020441.work.066-kees@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rtnetlink: do_setlink: Use true struct sockaddr | expand |
From: Kees Cook <kees@kernel.org> Date: Mon, 16 Dec 2024 18:04:45 -0800 > Instead of a heap allocation use a stack allocated struct sockaddr, as > dev_set_mac_address_user() is the consumer (which uses a classic > struct sockaddr). I remember Eric's feedback was to keep using heap instead of stack because rtnl_newlink() path already uses too much on stack. > Cap the copy to the minimum address size between > the incoming address and the traditional sa_data field itself. > > Putting "sa" on the stack means it will get a reused stack slot since > it is smaller than other existing single-scope stack variables (like > the vfinfo array). > > Signed-off-by: Kees Cook <kees@kernel.org> > --- > Cc: Eric Dumazet <edumazet@google.com> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Paolo Abeni <pabeni@redhat.com> > Cc: Ido Schimmel <idosch@nvidia.com> > Cc: Petr Machata <petrm@nvidia.com> > Cc: netdev@vger.kernel.org > --- > net/core/rtnetlink.c | 22 +++++++--------------- > 1 file changed, 7 insertions(+), 15 deletions(-) > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index ab5f201bf0ab..6da0edc0870d 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -3048,21 +3048,13 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev, > } > > if (tb[IFLA_ADDRESS]) { > - struct sockaddr *sa; > - int len; > - > - len = sizeof(sa_family_t) + max_t(size_t, dev->addr_len, > - sizeof(*sa)); > - sa = kmalloc(len, GFP_KERNEL); > - if (!sa) { > - err = -ENOMEM; > - goto errout; > - } > - sa->sa_family = dev->type; > - memcpy(sa->sa_data, nla_data(tb[IFLA_ADDRESS]), > - dev->addr_len); > - err = dev_set_mac_address_user(dev, sa, extack); > - kfree(sa); > + struct sockaddr sa = { }; > + > + /* dev_set_mac_address_user() uses a true struct sockaddr. */ > + sa.sa_family = dev->type; > + memcpy(sa.sa_data, nla_data(tb[IFLA_ADDRESS]), > + min(dev->addr_len, sizeof(sa.sa_data_min))); > + err = dev_set_mac_address_user(dev, &sa, extack); > if (err) > goto errout; > status |= DO_SETLINK_MODIFIED; > -- > 2.34.1
On December 16, 2024 6:41:56 PM PST, Kuniyuki Iwashima <kuniyu@amazon.com> wrote: >From: Kees Cook <kees@kernel.org> >Date: Mon, 16 Dec 2024 18:04:45 -0800 >> Instead of a heap allocation use a stack allocated struct sockaddr, as >> dev_set_mac_address_user() is the consumer (which uses a classic >> struct sockaddr). > >I remember Eric's feedback was to keep using heap instead of stack >because rtnl_newlink() path already uses too much on stack. See below... > > >> Cap the copy to the minimum address size between >> the incoming address and the traditional sa_data field itself. >> >> Putting "sa" on the stack means it will get a reused stack slot since >> it is smaller than other existing single-scope stack variables (like >> the vfinfo array). That's why I included the rationale above. (I.e. stack usage does not grow with this patch.) -Kees >> >> Signed-off-by: Kees Cook <kees@kernel.org> >> --- >> Cc: Eric Dumazet <edumazet@google.com> >> Cc: "David S. Miller" <davem@davemloft.net> >> Cc: Jakub Kicinski <kuba@kernel.org> >> Cc: Paolo Abeni <pabeni@redhat.com> >> Cc: Ido Schimmel <idosch@nvidia.com> >> Cc: Petr Machata <petrm@nvidia.com> >> Cc: netdev@vger.kernel.org >> --- >> net/core/rtnetlink.c | 22 +++++++--------------- >> 1 file changed, 7 insertions(+), 15 deletions(-) >> >> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c >> index ab5f201bf0ab..6da0edc0870d 100644 >> --- a/net/core/rtnetlink.c >> +++ b/net/core/rtnetlink.c >> @@ -3048,21 +3048,13 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev, >> } >> >> if (tb[IFLA_ADDRESS]) { >> - struct sockaddr *sa; >> - int len; >> - >> - len = sizeof(sa_family_t) + max_t(size_t, dev->addr_len, >> - sizeof(*sa)); >> - sa = kmalloc(len, GFP_KERNEL); >> - if (!sa) { >> - err = -ENOMEM; >> - goto errout; >> - } >> - sa->sa_family = dev->type; >> - memcpy(sa->sa_data, nla_data(tb[IFLA_ADDRESS]), >> - dev->addr_len); >> - err = dev_set_mac_address_user(dev, sa, extack); >> - kfree(sa); >> + struct sockaddr sa = { }; >> + >> + /* dev_set_mac_address_user() uses a true struct sockaddr. */ >> + sa.sa_family = dev->type; >> + memcpy(sa.sa_data, nla_data(tb[IFLA_ADDRESS]), >> + min(dev->addr_len, sizeof(sa.sa_data_min))); >> + err = dev_set_mac_address_user(dev, &sa, extack); >> if (err) >> goto errout; >> status |= DO_SETLINK_MODIFIED; >> -- >> 2.34.1 >
Le mar. 17 déc. 2024 à 03:05, Kees Cook <kees@kernel.org> a écrit : > > Instead of a heap allocation use a stack allocated struct sockaddr, as > dev_set_mac_address_user() is the consumer (which uses a classic > struct sockaddr). Cap the copy to the minimum address size between > the incoming address and the traditional sa_data field itself. Not sure what is a 'classic sockaddr' > > Putting "sa" on the stack means it will get a reused stack slot since > it is smaller than other existing single-scope stack variables (like > the vfinfo array). > > Signed-off-by: Kees Cook <kees@kernel.org> > --- > Cc: Eric Dumazet <edumazet@google.com> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Paolo Abeni <pabeni@redhat.com> > Cc: Ido Schimmel <idosch@nvidia.com> > Cc: Petr Machata <petrm@nvidia.com> > Cc: netdev@vger.kernel.org > --- > net/core/rtnetlink.c | 22 +++++++--------------- > 1 file changed, 7 insertions(+), 15 deletions(-) > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index ab5f201bf0ab..6da0edc0870d 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -3048,21 +3048,13 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev, > } > > if (tb[IFLA_ADDRESS]) { > - struct sockaddr *sa; > - int len; > - > - len = sizeof(sa_family_t) + max_t(size_t, dev->addr_len, > - sizeof(*sa)); > - sa = kmalloc(len, GFP_KERNEL); > - if (!sa) { > - err = -ENOMEM; > - goto errout; > - } > - sa->sa_family = dev->type; > - memcpy(sa->sa_data, nla_data(tb[IFLA_ADDRESS]), > - dev->addr_len); > - err = dev_set_mac_address_user(dev, sa, extack); > - kfree(sa); > + struct sockaddr sa = { }; > + > + /* dev_set_mac_address_user() uses a true struct sockaddr. */ > + sa.sa_family = dev->type; > + memcpy(sa.sa_data, nla_data(tb[IFLA_ADDRESS]), > + min(dev->addr_len, sizeof(sa.sa_data_min))); > + err = dev_set_mac_address_user(dev, &sa, extack); Have you added debug checks in dev_set_mac_address_user() to make sure dev->addr_len is always smaller than 14 ? I think we support devices with bigger addresses.
From: Kees Cook <kees@kernel.org> Date: Mon, 16 Dec 2024 23:53:46 -0800 > On December 16, 2024 6:41:56 PM PST, Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > >From: Kees Cook <kees@kernel.org> > >Date: Mon, 16 Dec 2024 18:04:45 -0800 > >> Instead of a heap allocation use a stack allocated struct sockaddr, as > >> dev_set_mac_address_user() is the consumer (which uses a classic > >> struct sockaddr). > > > >I remember Eric's feedback was to keep using heap instead of stack > >because rtnl_newlink() path already uses too much on stack. > > See below... > > > > > > >> Cap the copy to the minimum address size between > >> the incoming address and the traditional sa_data field itself. > >> > >> Putting "sa" on the stack means it will get a reused stack slot since > >> it is smaller than other existing single-scope stack variables (like > >> the vfinfo array). > > That's why I included the rationale above. (I.e. stack usage does not grow with this patch.) Ah okay, but I think we can't cap the address size to 14 bytes. MAX_ADDR_LEN is 32. Also, dev_set_mac_address_user() still uses dev->addr_len. > > -Kees > > >> > >> Signed-off-by: Kees Cook <kees@kernel.org> > >> --- > >> Cc: Eric Dumazet <edumazet@google.com> > >> Cc: "David S. Miller" <davem@davemloft.net> > >> Cc: Jakub Kicinski <kuba@kernel.org> > >> Cc: Paolo Abeni <pabeni@redhat.com> > >> Cc: Ido Schimmel <idosch@nvidia.com> > >> Cc: Petr Machata <petrm@nvidia.com> > >> Cc: netdev@vger.kernel.org > >> --- > >> net/core/rtnetlink.c | 22 +++++++--------------- > >> 1 file changed, 7 insertions(+), 15 deletions(-) > >> > >> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > >> index ab5f201bf0ab..6da0edc0870d 100644 > >> --- a/net/core/rtnetlink.c > >> +++ b/net/core/rtnetlink.c > >> @@ -3048,21 +3048,13 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev, > >> } > >> > >> if (tb[IFLA_ADDRESS]) { > >> - struct sockaddr *sa; > >> - int len; > >> - > >> - len = sizeof(sa_family_t) + max_t(size_t, dev->addr_len, > >> - sizeof(*sa)); > >> - sa = kmalloc(len, GFP_KERNEL); > >> - if (!sa) { > >> - err = -ENOMEM; > >> - goto errout; > >> - } > >> - sa->sa_family = dev->type; > >> - memcpy(sa->sa_data, nla_data(tb[IFLA_ADDRESS]), > >> - dev->addr_len); > >> - err = dev_set_mac_address_user(dev, sa, extack); > >> - kfree(sa); > >> + struct sockaddr sa = { }; > >> + > >> + /* dev_set_mac_address_user() uses a true struct sockaddr. */ > >> + sa.sa_family = dev->type; > >> + memcpy(sa.sa_data, nla_data(tb[IFLA_ADDRESS]), > >> + min(dev->addr_len, sizeof(sa.sa_data_min))); > >> + err = dev_set_mac_address_user(dev, &sa, extack); > >> if (err) > >> goto errout; > >> status |= DO_SETLINK_MODIFIED; > >> -- > >> 2.34.1 > > > > -- > Kees Cook
On December 17, 2024 12:03:35 AM PST, Kuniyuki Iwashima <kuniyu@amazon.com> wrote: >From: Kees Cook <kees@kernel.org> >Date: Mon, 16 Dec 2024 23:53:46 -0800 >> On December 16, 2024 6:41:56 PM PST, Kuniyuki Iwashima <kuniyu@amazon.com> wrote: >> >From: Kees Cook <kees@kernel.org> >> >Date: Mon, 16 Dec 2024 18:04:45 -0800 >> >> Instead of a heap allocation use a stack allocated struct sockaddr, as >> >> dev_set_mac_address_user() is the consumer (which uses a classic >> >> struct sockaddr). >> > >> >I remember Eric's feedback was to keep using heap instead of stack >> >because rtnl_newlink() path already uses too much on stack. >> >> See below... >> >> > >> > >> >> Cap the copy to the minimum address size between >> >> the incoming address and the traditional sa_data field itself. >> >> >> >> Putting "sa" on the stack means it will get a reused stack slot since >> >> it is smaller than other existing single-scope stack variables (like >> >> the vfinfo array). >> >> That's why I included the rationale above. (I.e. stack usage does not grow with this patch.) > >Ah okay, but I think we can't cap the address size to 14 >bytes. MAX_ADDR_LEN is 32. > >Also, dev_set_mac_address_user() still uses dev->addr_len. Oh, hrm, yes, that's true. I had audited callers of dev_set_mac_address(), but I think I must have missed callers of dev_set_mac_address_user(). Ugh. :( Let me take another look at this... -Kees > > >> >> -Kees >> >> >> >> >> Signed-off-by: Kees Cook <kees@kernel.org> >> >> --- >> >> Cc: Eric Dumazet <edumazet@google.com> >> >> Cc: "David S. Miller" <davem@davemloft.net> >> >> Cc: Jakub Kicinski <kuba@kernel.org> >> >> Cc: Paolo Abeni <pabeni@redhat.com> >> >> Cc: Ido Schimmel <idosch@nvidia.com> >> >> Cc: Petr Machata <petrm@nvidia.com> >> >> Cc: netdev@vger.kernel.org >> >> --- >> >> net/core/rtnetlink.c | 22 +++++++--------------- >> >> 1 file changed, 7 insertions(+), 15 deletions(-) >> >> >> >> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c >> >> index ab5f201bf0ab..6da0edc0870d 100644 >> >> --- a/net/core/rtnetlink.c >> >> +++ b/net/core/rtnetlink.c >> >> @@ -3048,21 +3048,13 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev, >> >> } >> >> >> >> if (tb[IFLA_ADDRESS]) { >> >> - struct sockaddr *sa; >> >> - int len; >> >> - >> >> - len = sizeof(sa_family_t) + max_t(size_t, dev->addr_len, >> >> - sizeof(*sa)); >> >> - sa = kmalloc(len, GFP_KERNEL); >> >> - if (!sa) { >> >> - err = -ENOMEM; >> >> - goto errout; >> >> - } >> >> - sa->sa_family = dev->type; >> >> - memcpy(sa->sa_data, nla_data(tb[IFLA_ADDRESS]), >> >> - dev->addr_len); >> >> - err = dev_set_mac_address_user(dev, sa, extack); >> >> - kfree(sa); >> >> + struct sockaddr sa = { }; >> >> + >> >> + /* dev_set_mac_address_user() uses a true struct sockaddr. */ >> >> + sa.sa_family = dev->type; >> >> + memcpy(sa.sa_data, nla_data(tb[IFLA_ADDRESS]), >> >> + min(dev->addr_len, sizeof(sa.sa_data_min))); >> >> + err = dev_set_mac_address_user(dev, &sa, extack); >> >> if (err) >> >> goto errout; >> >> status |= DO_SETLINK_MODIFIED; >> >> -- >> >> 2.34.1 >> > >> >> -- >> Kees Cook
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index ab5f201bf0ab..6da0edc0870d 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -3048,21 +3048,13 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev, } if (tb[IFLA_ADDRESS]) { - struct sockaddr *sa; - int len; - - len = sizeof(sa_family_t) + max_t(size_t, dev->addr_len, - sizeof(*sa)); - sa = kmalloc(len, GFP_KERNEL); - if (!sa) { - err = -ENOMEM; - goto errout; - } - sa->sa_family = dev->type; - memcpy(sa->sa_data, nla_data(tb[IFLA_ADDRESS]), - dev->addr_len); - err = dev_set_mac_address_user(dev, sa, extack); - kfree(sa); + struct sockaddr sa = { }; + + /* dev_set_mac_address_user() uses a true struct sockaddr. */ + sa.sa_family = dev->type; + memcpy(sa.sa_data, nla_data(tb[IFLA_ADDRESS]), + min(dev->addr_len, sizeof(sa.sa_data_min))); + err = dev_set_mac_address_user(dev, &sa, extack); if (err) goto errout; status |= DO_SETLINK_MODIFIED;
Instead of a heap allocation use a stack allocated struct sockaddr, as dev_set_mac_address_user() is the consumer (which uses a classic struct sockaddr). Cap the copy to the minimum address size between the incoming address and the traditional sa_data field itself. Putting "sa" on the stack means it will get a reused stack slot since it is smaller than other existing single-scope stack variables (like the vfinfo array). Signed-off-by: Kees Cook <kees@kernel.org> --- Cc: Eric Dumazet <edumazet@google.com> Cc: "David S. Miller" <davem@davemloft.net> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Paolo Abeni <pabeni@redhat.com> Cc: Ido Schimmel <idosch@nvidia.com> Cc: Petr Machata <petrm@nvidia.com> Cc: netdev@vger.kernel.org --- net/core/rtnetlink.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-)