| Message ID | 20240220102512.104452-1-kovalev@altlinux.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,ver.2] genetlink: fix possible use-after-free and null-ptr-deref in genl_dumpit() | expand |
On Tue, Feb 20, 2024 at 01:25:12PM +0300, kovalev@altlinux.org wrote: > From: Vasiliy Kovalev <kovalev@altlinux.org> > > The pernet operations structure for the subsystem must be registered > before registering the generic netlink family. IIRC, you pointed to a syzbot report on genetlink similar to gtp. Maybe add that tag here and get the robot to test this fix? I'd suggest to describe the scenario, which is: There is a race that allows netlink dump and walking on pernet data while such pernet data is not yet set up. Thanks. > Introduced in commit 134e63756d5f ("genetlink: make netns aware") > Signed-off-by: Vasiliy Kovalev <kovalev@altlinux.org> > --- > net/netlink/genetlink.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c > index 8c7af02f845400..20a7d792dd52ec 100644 > --- a/net/netlink/genetlink.c > +++ b/net/netlink/genetlink.c > @@ -1879,14 +1879,14 @@ static int __init genl_init(void) > { > int err; > > - err = genl_register_family(&genl_ctrl); > - if (err < 0) > - goto problem; > - > err = register_pernet_subsys(&genl_pernet_ops); > if (err) > goto problem; > > + err = genl_register_family(&genl_ctrl); > + if (err < 0) > + goto problem; > + > return 0; > > problem: > -- > 2.33.8 >
Hi Pablo, 20.02.2024 13:36, Pablo Neira Ayuso wrote: > On Tue, Feb 20, 2024 at 01:25:12PM +0300, kovalev@altlinux.org wrote: >> From: Vasiliy Kovalev <kovalev@altlinux.org> >> >> The pernet operations structure for the subsystem must be registered >> before registering the generic netlink family. > IIRC, you pointed to a syzbot report on genetlink similar to gtp. Yes, I was referring to the link https://lore.kernel.org/all/0000000000007549a6060f99544d@google.com/T/ , > > Maybe add that tag here and get the robot to test this fix? but since the syzbot does not have a reproducer, I cannot say for sure that this is exactly the problem that this patch fixes, since syzbot refers to the tipc_udp_nl_dump_remoteip function and suddenly there is another problem... > > I'd suggest to describe the scenario, which is: There is a race that > allows netlink dump and walking on pernet data while such pernet data > is not yet set up. > > Thanks.
On Tue, 20 Feb 2024 13:25:12 +0300 kovalev@altlinux.org wrote: > From: Vasiliy Kovalev <kovalev@altlinux.org> > > The pernet operations structure for the subsystem must be registered > before registering the generic netlink family. I think this one is incorrect, genetlink is what other families register _with_. It's special. Until it opens the socket in genl_pernet_init() nothing can reach it from user space. We should probably add a comment saying that it's special, I get the feeling other families ended up doing a copy & paste of genetlink..
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c index 8c7af02f845400..20a7d792dd52ec 100644 --- a/net/netlink/genetlink.c +++ b/net/netlink/genetlink.c @@ -1879,14 +1879,14 @@ static int __init genl_init(void) { int err; - err = genl_register_family(&genl_ctrl); - if (err < 0) - goto problem; - err = register_pernet_subsys(&genl_pernet_ops); if (err) goto problem; + err = genl_register_family(&genl_ctrl); + if (err < 0) + goto problem; + return 0; problem: