Re: [PATCH] genetlink: fix potencial use-after-free and null-ptr-deref in genl_dumpit()

[Simon Horman <horms@kernel.org>]

+ 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
> 
> 

-- 
pw-bot: changes-requested