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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87o76n85ml.fsf@nvidia.com>
Date: Wed, 24 Jul 2024 11:12:22 +0200
From: Petr Machata <petrm@...dia.com>
To: Eric Dumazet <edumazet@...gle.com>
CC: Petr Machata <petrm@...dia.com>, "David S. Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	<netdev@...r.kernel.org>, David Ahern <dsahern@...nel.org>,
	<mlxsw@...dia.com>, Ido Schimmel <idosch@...dia.com>
Subject: Re: [PATCH net] net: nexthop: Initialize all fields in dumped nexthops


Eric Dumazet <edumazet@...gle.com> writes:

> On Tue, Jul 23, 2024 at 7:41 PM Eric Dumazet <edumazet@...gle.com> wrote:
>>
>> On Tue, Jul 23, 2024 at 7:26 PM Eric Dumazet <edumazet@...gle.com> wrote:
>> >
>> > On Tue, Jul 23, 2024 at 6:50 PM Eric Dumazet <edumazet@...gle.com> wrote:
>> > >
>> > > On Tue, Jul 23, 2024 at 6:05 PM Petr Machata <petrm@...dia.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@...dia.com>
>> > > > Reviewed-by: Ido Schimmel <idosch@...dia.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@...gle.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.