[PATCH net] net: nexthop: Initialize all fields in dumped nexthops

[PATCH net] net: nexthop: Initialize all fields in dumped nexthops

[Petr Machata]

struct nexthop_grp contains two reserved fields that are not initialized by
nla_put_nh_group(), and carry garbage. This can be observed e.g. with
strace (edited for clarity):

    # ip nexthop add id 1 dev lo
    # ip nexthop add id 101 group 1
    # strace -e recvmsg ip nexthop get id 101
    ...
    recvmsg(... [{nla_len=12, nla_type=NHA_GROUP},
                 [{id=1, weight=0, resvd1=0x69, resvd2=0x67}]] ...) = 52

The fields are reserved and therefore not currently used. But as they are, they
leak kernel memory, and the fact they are not just zero complicates repurposing
of the fields for new ends. Initialize the full structure.

Fixes: 430a049190de ("nexthop: Add support for nexthop groups")
Signed-off-by: Petr Machata <petrm@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
---
 net/ipv4/nexthop.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index 535856b0f0ed..6b9787ee8601 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -888,9 +888,10 @@ static int nla_put_nh_group(struct sk_buff *skb, struct nexthop *nh,
 
 	p = nla_data(nla);
 	for (i = 0; i < nhg->num_nh; ++i) {
-		p->id = nhg->nh_entries[i].nh->id;
-		p->weight = nhg->nh_entries[i].weight - 1;
-		p += 1;
+		*p++ = (struct nexthop_grp) {
+			.id = nhg->nh_entries[i].nh->id,
+			.weight = nhg->nh_entries[i].weight - 1,
+		};
 	}
 
 	if (nhg->resilient && nla_put_nh_group_res(skb, nhg))
-- 
2.45.0

Re: [PATCH net] net: nexthop: Initialize all fields in dumped nexthops

[Eric Dumazet]

On Tue, Jul 23, 2024 at 6:05 PM Petr Machata <petrm@nvidia.com> wrote:
>
> struct nexthop_grp contains two reserved fields that are not initialized by
> nla_put_nh_group(), and carry garbage. This can be observed e.g. with
> strace (edited for clarity):
>
>     # ip nexthop add id 1 dev lo
>     # ip nexthop add id 101 group 1
>     # strace -e recvmsg ip nexthop get id 101
>     ...
>     recvmsg(... [{nla_len=12, nla_type=NHA_GROUP},
>                  [{id=1, weight=0, resvd1=0x69, resvd2=0x67}]] ...) = 52
>
> The fields are reserved and therefore not currently used. But as they are, they
> leak kernel memory, and the fact they are not just zero complicates repurposing
> of the fields for new ends. Initialize the full structure.
>
> Fixes: 430a049190de ("nexthop: Add support for nexthop groups")
> Signed-off-by: Petr Machata <petrm@nvidia.com>
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>

Interesting... not sure why syzbot did not catch this one.

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net] net: nexthop: Initialize all fields in dumped nexthops

[Eric Dumazet]

On Tue, Jul 23, 2024 at 6:50 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Jul 23, 2024 at 6:05 PM Petr Machata <petrm@nvidia.com> wrote:
> >
> > struct nexthop_grp contains two reserved fields that are not initialized by
> > nla_put_nh_group(), and carry garbage. This can be observed e.g. with
> > strace (edited for clarity):
> >
> >     # ip nexthop add id 1 dev lo
> >     # ip nexthop add id 101 group 1
> >     # strace -e recvmsg ip nexthop get id 101
> >     ...
> >     recvmsg(... [{nla_len=12, nla_type=NHA_GROUP},
> >                  [{id=1, weight=0, resvd1=0x69, resvd2=0x67}]] ...) = 52
> >
> > The fields are reserved and therefore not currently used. But as they are, they
> > leak kernel memory, and the fact they are not just zero complicates repurposing
> > of the fields for new ends. Initialize the full structure.
> >
> > Fixes: 430a049190de ("nexthop: Add support for nexthop groups")
> > Signed-off-by: Petr Machata <petrm@nvidia.com>
> > Reviewed-by: Ido Schimmel <idosch@nvidia.com>
>
> Interesting... not sure why syzbot did not catch this one.
>
> Reviewed-by: Eric Dumazet <edumazet@google.com>

Hmmm... Do we have the guarantee that the compiler initializes padding ?

AFAIK, padding at the end of the structure is not initialized.

Re: [PATCH net] net: nexthop: Initialize all fields in dumped nexthops

[Eric Dumazet]

On Tue, Jul 23, 2024 at 7:26 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Jul 23, 2024 at 6:50 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Tue, Jul 23, 2024 at 6:05 PM Petr Machata <petrm@nvidia.com> wrote:
> > >
> > > struct nexthop_grp contains two reserved fields that are not initialized by
> > > nla_put_nh_group(), and carry garbage. This can be observed e.g. with
> > > strace (edited for clarity):
> > >
> > >     # ip nexthop add id 1 dev lo
> > >     # ip nexthop add id 101 group 1
> > >     # strace -e recvmsg ip nexthop get id 101
> > >     ...
> > >     recvmsg(... [{nla_len=12, nla_type=NHA_GROUP},
> > >                  [{id=1, weight=0, resvd1=0x69, resvd2=0x67}]] ...) = 52
> > >
> > > The fields are reserved and therefore not currently used. But as they are, they
> > > leak kernel memory, and the fact they are not just zero complicates repurposing
> > > of the fields for new ends. Initialize the full structure.
> > >
> > > Fixes: 430a049190de ("nexthop: Add support for nexthop groups")
> > > Signed-off-by: Petr Machata <petrm@nvidia.com>
> > > Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> >
> > Interesting... not sure why syzbot did not catch this one.
> >
> > Reviewed-by: Eric Dumazet <edumazet@google.com>
>
> Hmmm... Do we have the guarantee that the compiler initializes padding ?
>
> AFAIK, padding at the end of the structure is not initialized.

I am asking this because syzbot found a similar issue in net/sched/act_ct.c

My current WIP is :

diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index 113b907da0f757af7be920cc9b3a1b1c769f5804..3ba8e7e739b58a96e66ca64d38bff758500df3e1
100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -44,6 +44,8 @@ static DEFINE_MUTEX(zones_mutex);
 struct zones_ht_key {
        struct net *net;
        u16 zone;
+       /* Note : pad[] must be the last field. */
+       u8  pad[];
 };

 struct tcf_ct_flow_table {
@@ -60,7 +62,7 @@ struct tcf_ct_flow_table {
 static const struct rhashtable_params zones_params = {
        .head_offset = offsetof(struct tcf_ct_flow_table, node),
        .key_offset = offsetof(struct tcf_ct_flow_table, key),
-       .key_len = sizeof_field(struct tcf_ct_flow_table, key),
+       .key_len = offsetof(struct zones_ht_key, pad),
        .automatic_shrinking = true,
 };

Re: [PATCH net] net: nexthop: Initialize all fields in dumped nexthops

[Eric Dumazet]

On Tue, Jul 23, 2024 at 7:41 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Jul 23, 2024 at 7:26 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Tue, Jul 23, 2024 at 6:50 PM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Tue, Jul 23, 2024 at 6:05 PM Petr Machata <petrm@nvidia.com> wrote:
> > > >
> > > > struct nexthop_grp contains two reserved fields that are not initialized by
> > > > nla_put_nh_group(), and carry garbage. This can be observed e.g. with
> > > > strace (edited for clarity):
> > > >
> > > >     # ip nexthop add id 1 dev lo
> > > >     # ip nexthop add id 101 group 1
> > > >     # strace -e recvmsg ip nexthop get id 101
> > > >     ...
> > > >     recvmsg(... [{nla_len=12, nla_type=NHA_GROUP},
> > > >                  [{id=1, weight=0, resvd1=0x69, resvd2=0x67}]] ...) = 52
> > > >
> > > > The fields are reserved and therefore not currently used. But as they are, they
> > > > leak kernel memory, and the fact they are not just zero complicates repurposing
> > > > of the fields for new ends. Initialize the full structure.
> > > >
> > > > Fixes: 430a049190de ("nexthop: Add support for nexthop groups")
> > > > Signed-off-by: Petr Machata <petrm@nvidia.com>
> > > > Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> > >
> > > Interesting... not sure why syzbot did not catch this one.
> > >
> > > Reviewed-by: Eric Dumazet <edumazet@google.com>
> >
> > Hmmm... Do we have the guarantee that the compiler initializes padding ?
> >
> > AFAIK, padding at the end of the structure is not initialized.
>
> I am asking this because syzbot found a similar issue in net/sched/act_ct.c
>
> My current WIP is :
>
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index 113b907da0f757af7be920cc9b3a1b1c769f5804..3ba8e7e739b58a96e66ca64d38bff758500df3e1
> 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -44,6 +44,8 @@ static DEFINE_MUTEX(zones_mutex);
>  struct zones_ht_key {
>         struct net *net;
>         u16 zone;
> +       /* Note : pad[] must be the last field. */
> +       u8  pad[];
>  };
>
>  struct tcf_ct_flow_table {
> @@ -60,7 +62,7 @@ struct tcf_ct_flow_table {
>  static const struct rhashtable_params zones_params = {
>         .head_offset = offsetof(struct tcf_ct_flow_table, node),
>         .key_offset = offsetof(struct tcf_ct_flow_table, key),
> -       .key_len = sizeof_field(struct tcf_ct_flow_table, key),
> +       .key_len = offsetof(struct zones_ht_key, pad),
>         .automatic_shrinking = true,
>  };

I guess your patch is fine, because the holes in struct nexthop_grp are named.

Re: [PATCH net] net: nexthop: Initialize all fields in dumped nexthops

[Petr Machata]

Eric Dumazet <edumazet@google.com> writes:

> On Tue, Jul 23, 2024 at 7:41 PM Eric Dumazet <edumazet@google.com> wrote:
>>
>> On Tue, Jul 23, 2024 at 7:26 PM Eric Dumazet <edumazet@google.com> wrote:
>> >
>> > On Tue, Jul 23, 2024 at 6:50 PM Eric Dumazet <edumazet@google.com> wrote:
>> > >
>> > > On Tue, Jul 23, 2024 at 6:05 PM Petr Machata <petrm@nvidia.com> wrote:
>> > > >
>> > > > struct nexthop_grp contains two reserved fields that are not initialized by
>> > > > nla_put_nh_group(), and carry garbage. This can be observed e.g. with
>> > > > strace (edited for clarity):
>> > > >
>> > > >     # ip nexthop add id 1 dev lo
>> > > >     # ip nexthop add id 101 group 1
>> > > >     # strace -e recvmsg ip nexthop get id 101
>> > > >     ...
>> > > >     recvmsg(... [{nla_len=12, nla_type=NHA_GROUP},
>> > > >                  [{id=1, weight=0, resvd1=0x69, resvd2=0x67}]] ...) = 52
>> > > >
>> > > > The fields are reserved and therefore not currently used. But as they are, they
>> > > > leak kernel memory, and the fact they are not just zero complicates repurposing
>> > > > of the fields for new ends. Initialize the full structure.
>> > > >
>> > > > Fixes: 430a049190de ("nexthop: Add support for nexthop groups")
>> > > > Signed-off-by: Petr Machata <petrm@nvidia.com>
>> > > > Reviewed-by: Ido Schimmel <idosch@nvidia.com>
>> > >
>> > > Interesting... not sure why syzbot did not catch this one.

Could it? I'm not sure of the exact syzcaller capabilities, but there
are no warnings, no splats etc. It just returns values.

>> > > Reviewed-by: Eric Dumazet <edumazet@google.com>
>> >
>> > Hmmm... Do we have the guarantee that the compiler initializes padding ?
>> >
>> > AFAIK, padding at the end of the structure is not initialized.
>>
>> I am asking this because syzbot found a similar issue in net/sched/act_ct.c
>>
>> My current WIP is :
>>
>> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
>> index 113b907da0f757af7be920cc9b3a1b1c769f5804..3ba8e7e739b58a96e66ca64d38bff758500df3e1
>> 100644
>> --- a/net/sched/act_ct.c
>> +++ b/net/sched/act_ct.c
>> @@ -44,6 +44,8 @@ static DEFINE_MUTEX(zones_mutex);
>>  struct zones_ht_key {
>>         struct net *net;
>>         u16 zone;
>> +       /* Note : pad[] must be the last field. */
>> +       u8  pad[];
>>  };
>>
>>  struct tcf_ct_flow_table {
>> @@ -60,7 +62,7 @@ struct tcf_ct_flow_table {
>>  static const struct rhashtable_params zones_params = {
>>         .head_offset = offsetof(struct tcf_ct_flow_table, node),
>>         .key_offset = offsetof(struct tcf_ct_flow_table, key),
>> -       .key_len = sizeof_field(struct tcf_ct_flow_table, key),
>> +       .key_len = offsetof(struct zones_ht_key, pad),
>>         .automatic_shrinking = true,
>>  };
>
> I guess your patch is fine, because the holes in struct nexthop_grp are named.

Yep, that's it. It's not padding, it's just fields.

Otherwise AFAIK padding is unspecified in general. For these partial
structure initializations (i.e. where some fields are omitted), at least
GCC as of recently has initialized padding to zeroes, but it's not
guaranteed.

Re: [PATCH net] net: nexthop: Initialize all fields in dumped nexthops

[Eric Dumazet]

On Wed, Jul 24, 2024 at 12:09 PM Petr Machata <petrm@nvidia.com> wrote:
>
>
> Eric Dumazet <edumazet@google.com> writes:
>
> > On Tue, Jul 23, 2024 at 7:41 PM Eric Dumazet <edumazet@google.com> wrote:
> >>
> >> On Tue, Jul 23, 2024 at 7:26 PM Eric Dumazet <edumazet@google.com> wrote:
> >> >
> >> > On Tue, Jul 23, 2024 at 6:50 PM Eric Dumazet <edumazet@google.com> wrote:
> >> > >
> >> > > On Tue, Jul 23, 2024 at 6:05 PM Petr Machata <petrm@nvidia.com> wrote:
> >> > > >
> >> > > > struct nexthop_grp contains two reserved fields that are not initialized by
> >> > > > nla_put_nh_group(), and carry garbage. This can be observed e.g. with
> >> > > > strace (edited for clarity):
> >> > > >
> >> > > >     # ip nexthop add id 1 dev lo
> >> > > >     # ip nexthop add id 101 group 1
> >> > > >     # strace -e recvmsg ip nexthop get id 101
> >> > > >     ...
> >> > > >     recvmsg(... [{nla_len=12, nla_type=NHA_GROUP},
> >> > > >                  [{id=1, weight=0, resvd1=0x69, resvd2=0x67}]] ...) = 52
> >> > > >
> >> > > > The fields are reserved and therefore not currently used. But as they are, they
> >> > > > leak kernel memory, and the fact they are not just zero complicates repurposing
> >> > > > of the fields for new ends. Initialize the full structure.
> >> > > >
> >> > > > Fixes: 430a049190de ("nexthop: Add support for nexthop groups")
> >> > > > Signed-off-by: Petr Machata <petrm@nvidia.com>
> >> > > > Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> >> > >
> >> > > Interesting... not sure why syzbot did not catch this one.
>
> Could it? I'm not sure of the exact syzcaller capabilities, but there
> are no warnings, no splats etc. It just returns values.

Yes, KMSAN can detect such things (uninit-value)

Re: [PATCH net] net: nexthop: Initialize all fields in dumped nexthops

[Petr Machata]

Eric Dumazet <edumazet@google.com> writes:

> On Wed, Jul 24, 2024 at 12:09 PM Petr Machata <petrm@nvidia.com> wrote:
>>
>>
>> Eric Dumazet <edumazet@google.com> writes:
>>
>> > On Tue, Jul 23, 2024 at 7:41 PM Eric Dumazet <edumazet@google.com> wrote:
>> >>
>> >> On Tue, Jul 23, 2024 at 7:26 PM Eric Dumazet <edumazet@google.com> wrote:
>> >> >
>> >> > On Tue, Jul 23, 2024 at 6:50 PM Eric Dumazet <edumazet@google.com> wrote:
>> >> > >
>> >> > > On Tue, Jul 23, 2024 at 6:05 PM Petr Machata <petrm@nvidia.com> wrote:
>> >> > > >
>> >> > > > struct nexthop_grp contains two reserved fields that are not initialized by
>> >> > > > nla_put_nh_group(), and carry garbage. This can be observed e.g. with
>> >> > > > strace (edited for clarity):
>> >> > > >
>> >> > > >     # ip nexthop add id 1 dev lo
>> >> > > >     # ip nexthop add id 101 group 1
>> >> > > >     # strace -e recvmsg ip nexthop get id 101
>> >> > > >     ...
>> >> > > >     recvmsg(... [{nla_len=12, nla_type=NHA_GROUP},
>> >> > > >                  [{id=1, weight=0, resvd1=0x69, resvd2=0x67}]] ...) = 52
>> >> > > >
>> >> > > > The fields are reserved and therefore not currently used. But as they are, they
>> >> > > > leak kernel memory, and the fact they are not just zero complicates repurposing
>> >> > > > of the fields for new ends. Initialize the full structure.
>> >> > > >
>> >> > > > Fixes: 430a049190de ("nexthop: Add support for nexthop groups")
>> >> > > > Signed-off-by: Petr Machata <petrm@nvidia.com>
>> >> > > > Reviewed-by: Ido Schimmel <idosch@nvidia.com>
>> >> > >
>> >> > > Interesting... not sure why syzbot did not catch this one.
>>
>> Could it? I'm not sure of the exact syzcaller capabilities, but there
>> are no warnings, no splats etc. It just returns values.
>
> Yes, KMSAN can detect such things (uninit-value)

But that would involve a splat. There's no splat with this issue, even
though I'm testing on a CONFIG_HAVE_ARCH_KMSAN kernel.

Re: [PATCH net] net: nexthop: Initialize all fields in dumped nexthops

[Petr Machata]

Petr Machata <petrm@nvidia.com> writes:

> Eric Dumazet <edumazet@google.com> writes:
>
>> On Wed, Jul 24, 2024 at 12:09 PM Petr Machata <petrm@nvidia.com> wrote:
>>>
>>>
>>> Eric Dumazet <edumazet@google.com> writes:
>>>
>>> > On Tue, Jul 23, 2024 at 7:41 PM Eric Dumazet <edumazet@google.com> wrote:
>>> >>
>>> >> On Tue, Jul 23, 2024 at 7:26 PM Eric Dumazet <edumazet@google.com> wrote:
>>> >> >
>>> >> > On Tue, Jul 23, 2024 at 6:50 PM Eric Dumazet <edumazet@google.com> wrote:
>>> >> > >
>>> >> > > On Tue, Jul 23, 2024 at 6:05 PM Petr Machata <petrm@nvidia.com> wrote:
>>> >> > > >
>>> >> > > > struct nexthop_grp contains two reserved fields that are not initialized by
>>> >> > > > nla_put_nh_group(), and carry garbage. This can be observed e.g. with
>>> >> > > > strace (edited for clarity):
>>> >> > > >
>>> >> > > >     # ip nexthop add id 1 dev lo
>>> >> > > >     # ip nexthop add id 101 group 1
>>> >> > > >     # strace -e recvmsg ip nexthop get id 101
>>> >> > > >     ...
>>> >> > > >     recvmsg(... [{nla_len=12, nla_type=NHA_GROUP},
>>> >> > > >                  [{id=1, weight=0, resvd1=0x69, resvd2=0x67}]] ...) = 52
>>> >> > > >
>>> >> > > > The fields are reserved and therefore not currently used. But as they are, they
>>> >> > > > leak kernel memory, and the fact they are not just zero complicates repurposing
>>> >> > > > of the fields for new ends. Initialize the full structure.
>>> >> > > >
>>> >> > > > Fixes: 430a049190de ("nexthop: Add support for nexthop groups")
>>> >> > > > Signed-off-by: Petr Machata <petrm@nvidia.com>
>>> >> > > > Reviewed-by: Ido Schimmel <idosch@nvidia.com>
>>> >> > >
>>> >> > > Interesting... not sure why syzbot did not catch this one.
>>>
>>> Could it? I'm not sure of the exact syzcaller capabilities, but there
>>> are no warnings, no splats etc. It just returns values.
>>
>> Yes, KMSAN can detect such things (uninit-value)
>
> But that would involve a splat. There's no splat with this issue, even
> though I'm testing on a CONFIG_HAVE_ARCH_KMSAN kernel.

OK, Ido tells me this is just the "it's available on this arch" option
and the actual option apparently needs clang. So disregard what I said.

Re: [PATCH net] net: nexthop: Initialize all fields in dumped nexthops

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Tue, 23 Jul 2024 18:04:16 +0200 you wrote:
> struct nexthop_grp contains two reserved fields that are not initialized by
> nla_put_nh_group(), and carry garbage. This can be observed e.g. with
> strace (edited for clarity):
> 
>     # ip nexthop add id 1 dev lo
>     # ip nexthop add id 101 group 1
>     # strace -e recvmsg ip nexthop get id 101
>     ...
>     recvmsg(... [{nla_len=12, nla_type=NHA_GROUP},
>                  [{id=1, weight=0, resvd1=0x69, resvd2=0x67}]] ...) = 52
> 
> [...]

Here is the summary with links:
  - [net] net: nexthop: Initialize all fields in dumped nexthops
    https://git.kernel.org/netdev/net/c/6d745cd0e972

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html