| Message ID | 20240215202309.29723-1-kovalev@altlinux.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | genetlink: fix potencial use-after-free and null-ptr-deref in genl_dumpit() | expand |
+ Jiri Pirko <jiri@resnulli.us> David Lebrun <david.lebrun@uclouvain.be> On Thu, Feb 15, 2024 at 11:23:09PM +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. > > Fixes: 134e63756d5f ("genetlink: make netns aware") Hi Vasiliy, A Fixes tag implies that this is a bug fix. So I think some explanation is warranted of what, user-visible, problem this resolves. In that case the patch should be targeted at net. Which means it should be based on that tree and have a net annotation in the subject Subject: [PATCH net] ... Alternatively, the Fixes tag should be dropped and some explanation should be provided of why the structure needs to be registered before the family. In this case, if you wish to refer to the patch where the problem (but not bug) was introduced you can use something like the following. It is just the Fixes tag that has a special meaning. Introduced in 134e63756d5f ("genetlink: make netns aware") I think the above comments also apply to: - [PATCH] ipv6: sr: fix possible use-after-free and null-ptr-deref https://lore.kernel.org/all/20240215202717.29815-1-kovalev@altlinux.org/ - [PATCH] devlink: fix possible use-after-free and memory leaks in devlink_init() https://lore.kernel.org/all/20240215203400.29976-1-kovalev@altlinux.org/ And as these patches seem to try to fix the same problem in different places, all under Networking, I would suggest that if you do repost, they are combined into a patch series (3 patches in the same series). But I do wonder, how such an apparently fundamental problem has been present for so long in what I assume to be well exercised code. Also, potential is misspelt in the subject of this patch. > Signed-off-by: Vasiliy Kovalev <kovalev@altlinux.org> > --- > net/netlink/genetlink.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c > index 8c7af02f845400..3bd628675a569f 100644 > --- a/net/netlink/genetlink.c > +++ b/net/netlink/genetlink.c > @@ -1879,14 +1879,16 @@ 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) { > + unregister_pernet_subsys(&genl_pernet_ops); > + goto problem; The problem label calls panic(). As noted elsewhere [1] there is no expectation of recovering from panic(), so there is no need to clean up here. [1] https://lore.kernel.org/all/CANn89i+TNVtk8UT1+2QeeKHR-b6AQoopdxpcqcbNVOp9+JYSYw@mail.gmail.com/ > + } > + > return 0; > > problem: > -- > 2.33.8 > >
+ Pablo Neira Ayuso <pablo@netfilter.org> 19.02.2024 14:32, Simon Horman wrote: > + Jiri Pirko <jiri@resnulli.us> > David Lebrun <david.lebrun@uclouvain.be> > > On Thu, Feb 15, 2024 at 11:23:09PM +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. >> >> Fixes: 134e63756d5f ("genetlink: make netns aware") > Hi Vasiliy, > > A Fixes tag implies that this is a bug fix. > So I think some explanation is warranted of what, user-visible, > problem this resolves. > > In that case the patch should be targeted at net. > Which means it should be based on that tree and have a net annotation > in the subject > > Subject: [PATCH net] ... > > Alternatively, the Fixes tag should be dropped and some explanation > should be provided of why the structure needs to be registered before > the family. > > In this case, if you wish to refer to the patch where the problem (but not > bug) was introduced you can use something like the following. > It is just the Fixes tag that has a special meaning. > > Introduced in 134e63756d5f ("genetlink: make netns aware") > > I think the above comments also apply to: > > - [PATCH] ipv6: sr: fix possible use-after-free and null-ptr-deref > https://lore.kernel.org/all/20240215202717.29815-1-kovalev@altlinux.org/ > > - [PATCH] devlink: fix possible use-after-free and memory leaks in devlink_init() > https://lore.kernel.org/all/20240215203400.29976-1-kovalev@altlinux.org/ > > And as these patches seem to try to fix the same problem in different > places, all under Networking, I would suggest that if you do repost, > they are combined into a patch series (3 patches in the same series). > > But I do wonder, how such an apparently fundamental problem has been > present for so long in what I assume to be well exercised code. Hi Simon, The history of these changes began with the crash fix in the gtp module [1] A solution to the problem was found [2] and Pablo Neruda Ayuso suggested fixing similar sections of code if they might have the same problem. I have sent patches, but do not have reproducers, relying on drawing attention to the problem. [1] https://lore.kernel.org/lkml/20240124101404.161655-1-kovalev@altlinux.org/T/ [2] https://lore.kernel.org/netdev/20240214162733.34214-1-kovalev@altlinux.org/T/#u
On Mon, Feb 19, 2024 at 03:15:52PM +0300, kovalev@altlinux.org wrote: > + Pablo Neira Ayuso <pablo@netfilter.org> > > 19.02.2024 14:32, Simon Horman wrote: > > + Jiri Pirko <jiri@resnulli.us> > > David Lebrun <david.lebrun@uclouvain.be> > > > > On Thu, Feb 15, 2024 at 11:23:09PM +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. > > > > > > Fixes: 134e63756d5f ("genetlink: make netns aware") > > Hi Vasiliy, > > > > A Fixes tag implies that this is a bug fix. > > So I think some explanation is warranted of what, user-visible, > > problem this resolves. > > > > In that case the patch should be targeted at net. > > Which means it should be based on that tree and have a net annotation > > in the subject > > > > Subject: [PATCH net] ... > > > > Alternatively, the Fixes tag should be dropped and some explanation > > should be provided of why the structure needs to be registered before > > the family. > > > > In this case, if you wish to refer to the patch where the problem (but not > > bug) was introduced you can use something like the following. > > It is just the Fixes tag that has a special meaning. > > > > Introduced in 134e63756d5f ("genetlink: make netns aware") > > > > I think the above comments also apply to: > > > > - [PATCH] ipv6: sr: fix possible use-after-free and null-ptr-deref > > https://lore.kernel.org/all/20240215202717.29815-1-kovalev@altlinux.org/ > > > > - [PATCH] devlink: fix possible use-after-free and memory leaks in devlink_init() > > https://lore.kernel.org/all/20240215203400.29976-1-kovalev@altlinux.org/ > > > > And as these patches seem to try to fix the same problem in different > > places, all under Networking, I would suggest that if you do repost, > > they are combined into a patch series (3 patches in the same series). > > > > But I do wonder, how such an apparently fundamental problem has been > > present for so long in what I assume to be well exercised code. > > Hi Simon, > > The history of these changes began with the crash fix in the gtp module [1] > > A solution to the problem was found [2] and Pablo Neruda Ayuso suggested > fixing similar > > sections of code if they might have the same problem. > > I have sent patches, but do not have reproducers, relying on drawing > attention to the problem. Thanks Vasiliy, I think it would be worth adding some text along those lines to the commit messages for the patches you have posted. > > > [1] > https://lore.kernel.org/lkml/20240124101404.161655-1-kovalev@altlinux.org/T/ > > [2] https://lore.kernel.org/netdev/20240214162733.34214-1-kovalev@altlinux.org/T/#u > > -- > Thanks, > Vasiliy Kovalev >
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c index 8c7af02f845400..3bd628675a569f 100644 --- a/net/netlink/genetlink.c +++ b/net/netlink/genetlink.c @@ -1879,14 +1879,16 @@ 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) { + unregister_pernet_subsys(&genl_pernet_ops); + goto problem; + } + return 0; problem: