| Message ID | 8b06dd4ec057d912ca3947bacf15529272dea796.1721749627.git.petrm@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6d745cd0e9720282cd291d36b9db528aea18add2 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: nexthop: Initialize all fields in dumped nexthops | expand |
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>
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.
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, };
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.
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.
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)
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!
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.
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.
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))
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(-)