| Message ID | 20241104222513.3469025-3-kees@kernel.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | sockaddr usage removal | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply, async |
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;
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(-)