* [PATCH] rtnetlink: do_setlink: Use true struct sockaddr
@ 2024-12-17 2:04 Kees Cook
2024-12-17 2:41 ` Kuniyuki Iwashima
2024-12-17 8:03 ` ericnetdev dumazet
0 siblings, 2 replies; 6+ messages in thread
From: Kees Cook @ 2024-12-17 2:04 UTC (permalink / raw)
To: Eric Dumazet
Cc: Kees Cook, David S. Miller, Jakub Kicinski, Paolo Abeni,
Ido Schimmel, Petr Machata, netdev, Simon Horman,
Kuniyuki Iwashima, linux-kernel, linux-hardening
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(-)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] rtnetlink: do_setlink: Use true struct sockaddr
2024-12-17 2:04 [PATCH] rtnetlink: do_setlink: Use true struct sockaddr Kees Cook
@ 2024-12-17 2:41 ` Kuniyuki Iwashima
2024-12-17 7:53 ` Kees Cook
2024-12-17 8:03 ` ericnetdev dumazet
1 sibling, 1 reply; 6+ messages in thread
From: Kuniyuki Iwashima @ 2024-12-17 2:41 UTC (permalink / raw)
To: kees
Cc: davem, edumazet, horms, idosch, kuba, kuniyu, linux-hardening,
linux-kernel, netdev, pabeni, petrm
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] rtnetlink: do_setlink: Use true struct sockaddr
2024-12-17 2:41 ` Kuniyuki Iwashima
@ 2024-12-17 7:53 ` Kees Cook
2024-12-17 8:03 ` Kuniyuki Iwashima
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2024-12-17 7:53 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: davem, edumazet, horms, idosch, kuba, kuniyu, linux-hardening,
linux-kernel, netdev, pabeni, petrm
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
>
--
Kees Cook
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] rtnetlink: do_setlink: Use true struct sockaddr
2024-12-17 7:53 ` Kees Cook
@ 2024-12-17 8:03 ` Kuniyuki Iwashima
2024-12-17 9:33 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Kuniyuki Iwashima @ 2024-12-17 8:03 UTC (permalink / raw)
To: kees
Cc: davem, edumazet, horms, idosch, kuba, kuniyu, linux-hardening,
linux-kernel, netdev, pabeni, petrm
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] rtnetlink: do_setlink: Use true struct sockaddr
2024-12-17 8:03 ` Kuniyuki Iwashima
@ 2024-12-17 9:33 ` Kees Cook
0 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2024-12-17 9:33 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: davem, edumazet, horms, idosch, kuba, kuniyu, linux-hardening,
linux-kernel, netdev, pabeni, petrm
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
--
Kees Cook
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rtnetlink: do_setlink: Use true struct sockaddr
2024-12-17 2:04 [PATCH] rtnetlink: do_setlink: Use true struct sockaddr Kees Cook
2024-12-17 2:41 ` Kuniyuki Iwashima
@ 2024-12-17 8:03 ` ericnetdev dumazet
1 sibling, 0 replies; 6+ messages in thread
From: ericnetdev dumazet @ 2024-12-17 8:03 UTC (permalink / raw)
To: Kees Cook
Cc: Eric Dumazet, David S. Miller, Jakub Kicinski, Paolo Abeni,
Ido Schimmel, Petr Machata, netdev, Simon Horman,
Kuniyuki Iwashima, linux-kernel, linux-hardening
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-12-17 9:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-17 2:04 [PATCH] rtnetlink: do_setlink: Use true struct sockaddr Kees Cook
2024-12-17 2:41 ` Kuniyuki Iwashima
2024-12-17 7:53 ` Kees Cook
2024-12-17 8:03 ` Kuniyuki Iwashima
2024-12-17 9:33 ` Kees Cook
2024-12-17 8:03 ` ericnetdev dumazet
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).