| Message ID | 20241104222513.3469025-3-kees@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | sockaddr usage removal | expand |
On 11/4/24 11:25 PM, Kees Cook wrote: > Instead of a heap allocation use a stack allocated sockaddr_storage to > support arbitrary length addr_len value (but bounds check it against the > maximum address length). > > Signed-off-by: Kees Cook <kees@kernel.org> > --- > net/core/rtnetlink.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index f0a520987085..eddd10b74f06 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -2839,21 +2839,17 @@ static int do_setlink(const struct sk_buff *skb, > } > > if (tb[IFLA_ADDRESS]) { > - struct sockaddr *sa; > - int len; > + struct sockaddr_storage addr; > + struct sockaddr *sa = (struct sockaddr *)&addr; We already use too much stack space. Please move addr into struct rtnl_newlink_tbs ? > > - len = sizeof(sa_family_t) + max_t(size_t, dev->addr_len, > - sizeof(*sa)); > - sa = kmalloc(len, GFP_KERNEL); > - if (!sa) { > + if (dev->addr_len > sizeof(addr.__data)) { > err = -ENOMEM; > goto errout; > } > sa->sa_family = dev->type; > - memcpy(sa->sa_data, nla_data(tb[IFLA_ADDRESS]), > + memcpy(addr.__data, nla_data(tb[IFLA_ADDRESS]), > dev->addr_len); > err = dev_set_mac_address_user(dev, sa, extack); > - kfree(sa); > if (err) > goto errout; > status |= DO_SETLINK_MODIFIED;
On Tue, Nov 05, 2024 at 11:59:49AM +0100, Eric Dumazet wrote: > > On 11/4/24 11:25 PM, Kees Cook wrote: > > Instead of a heap allocation use a stack allocated sockaddr_storage to > > support arbitrary length addr_len value (but bounds check it against the > > maximum address length). > > > > Signed-off-by: Kees Cook <kees@kernel.org> > > --- > > net/core/rtnetlink.c | 12 ++++-------- > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > > index f0a520987085..eddd10b74f06 100644 > > --- a/net/core/rtnetlink.c > > +++ b/net/core/rtnetlink.c > > @@ -2839,21 +2839,17 @@ static int do_setlink(const struct sk_buff *skb, > > } > > if (tb[IFLA_ADDRESS]) { > > - struct sockaddr *sa; > > - int len; > > + struct sockaddr_storage addr; > > + struct sockaddr *sa = (struct sockaddr *)&addr; > > We already use too much stack space. I'm finally coming back to this. I was over-thinking: this only calls dev_set_mac_address_user() so it really is a true struct sockaddr. > Please move addr into struct rtnl_newlink_tbs ? I couldn't figure out how that was meant to work -- I didn't see a struct rtnl_newlink_tbs instance near this routine. Regardless, I can just tweak the length and leave it heap allocated. -Kees
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index f0a520987085..eddd10b74f06 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -2839,21 +2839,17 @@ static int do_setlink(const struct sk_buff *skb, } if (tb[IFLA_ADDRESS]) { - struct sockaddr *sa; - int len; + struct sockaddr_storage addr; + struct sockaddr *sa = (struct sockaddr *)&addr; - len = sizeof(sa_family_t) + max_t(size_t, dev->addr_len, - sizeof(*sa)); - sa = kmalloc(len, GFP_KERNEL); - if (!sa) { + if (dev->addr_len > sizeof(addr.__data)) { err = -ENOMEM; goto errout; } sa->sa_family = dev->type; - memcpy(sa->sa_data, nla_data(tb[IFLA_ADDRESS]), + memcpy(addr.__data, nla_data(tb[IFLA_ADDRESS]), dev->addr_len); err = dev_set_mac_address_user(dev, sa, extack); - kfree(sa); if (err) goto errout; status |= DO_SETLINK_MODIFIED;
Instead of a heap allocation use a stack allocated sockaddr_storage to support arbitrary length addr_len value (but bounds check it against the maximum address length). Signed-off-by: Kees Cook <kees@kernel.org> --- net/core/rtnetlink.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)