netdev - Re: [PATCH ver.2] gtp: fix use-after-free and null-ptr-deref in gtp_genl_dump

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: <ZczvJKETNyFE5Glm@calendula>
Date: Wed, 14 Feb 2024 17:49:40 +0100
From: Pablo Neira Ayuso <pablo@...filter.org>
To: kovalev@...linux.org
Cc: linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	edumazet@...gle.com, laforge@...monks.org, davem@...emloft.net,
	kuba@...nel.org, pabeni@...hat.com, nickel@...linux.org,
	oficerovas@...linux.org, dutyrok@...linux.org,
	stable@...r.kernel.org
Subject: Re: [PATCH ver.2] gtp: fix use-after-free and null-ptr-deref in
 gtp_genl_dump_pdp()

On Wed, Feb 14, 2024 at 07:27:33PM +0300, kovalev@...linux.org wrote:
> From: Vasiliy Kovalev <kovalev@...linux.org>
> 
> The gtp_net_ops pernet operations structure for the subsystem must be
> registered before registering the generic netlink family.

Thanks for finding a remedy for this.

If your fix is correct, (I didn't test your patch yet) then maybe this
needs to be fixed in a few more spots in the tree?

net/devlink/core.c-static int __init devlink_init(void)
net/devlink/core.c-{
net/devlink/core.c-     int err;
net/devlink/core.c-
net/devlink/core.c-     err = genl_register_family(&devlink_nl_family);
net/devlink/core.c-     if (err)
net/devlink/core.c-             goto out;
net/devlink/core.c:     err = register_pernet_subsys(&devlink_pernet_ops);
net/devlink/core.c-     if (err)

net/handshake/netlink.c-        ret = genl_register_family(&handshake_nl_family);
net/handshake/netlink.c-        if (ret) {
net/handshake/netlink.c-                pr_warn("handshake: netlink registration failed (%d)\n", ret);
net/handshake/netlink.c-                handshake_req_hash_destroy();
net/handshake/netlink.c-                return ret;
net/handshake/netlink.c-        }
net/handshake/netlink.c-
net/handshake/netlink.c-        /*
net/handshake/netlink.c-         * ORDER: register_pernet_subsys must be done last.
net/handshake/netlink.c-         *
net/handshake/netlink.c-         *      If initialization does not make it past pernet_subsys
net/handshake/netlink.c-         *      registration, then handshake_net_id will remain 0. That
net/handshake/netlink.c-         *      shunts the handshake consumer API to return ENOTSUPP
net/handshake/netlink.c-         *      to prevent it from dereferencing something that hasn't
net/handshake/netlink.c-         *      been allocated.
net/handshake/netlink.c-         */
net/handshake/netlink.c:        ret = register_pernet_subsys(&handshake_genl_net_ops);

net/ipv4/tcp_metrics.c: ret = register_pernet_subsys(&tcp_net_metrics_ops);
net/ipv4/tcp_metrics.c- if (ret < 0)
net/ipv4/tcp_metrics.c-         panic("Could not register tcp_net_metrics_ops\n");
net/ipv4/tcp_metrics.c-
net/ipv4/tcp_metrics.c- ret = genl_register_family(&tcp_metrics_nl_family);
net/ipv4/tcp_metrics.c- if (ret < 0)
net/ipv4/tcp_metrics.c-         panic("Could not register tcp_metrics generic netlink\n");
net/ipv4/tcp_metrics.c-}

net/ipv6/ioam6.c-int __init ioam6_init(void)
net/ipv6/ioam6.c-{
net/ipv6/ioam6.c:       int err = register_pernet_subsys(&ioam6_net_ops);
net/ipv6/ioam6.c-       if (err)
net/ipv6/ioam6.c-               goto out;
net/ipv6/ioam6.c-
net/ipv6/ioam6.c-       err = genl_register_family(&ioam6_genl_family);
net/ipv6/ioam6.c-       if (err)
net/ipv6/ioam6.c-               goto out_unregister_pernet_subsys;

net/ipv6/seg6.c-        err = genl_register_family(&seg6_genl_family);
net/ipv6/seg6.c-        if (err)
net/ipv6/seg6.c-                goto out;
net/ipv6/seg6.c-
net/ipv6/seg6.c:        err = register_pernet_subsys(&ip6_segments_ops);
net/ipv6/seg6.c-        if (err)
net/ipv6/seg6.c-                goto out_unregister_genl;

net/netlink/genetlink.c-        err = genl_register_family(&genl_ctrl);
net/netlink/genetlink.c-        if (err < 0)
net/netlink/genetlink.c-                goto problem;
net/netlink/genetlink.c-
net/netlink/genetlink.c:        err = register_pernet_subsys(&genl_pernet_ops);
net/netlink/genetlink.c-        if (err)
net/netlink/genetlink.c-                goto problem;

> Syzkaller hit 'general protection fault in gtp_genl_dump_pdp' bug:
[...]
> diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
> index b1919278e931f4..2129ae42c70304 100644
> --- a/drivers/net/gtp.c
> +++ b/drivers/net/gtp.c
> @@ -1907,20 +1907,20 @@ static int __init gtp_init(void)
>  	if (err < 0)
>  		goto error_out;
>  
> -	err = genl_register_family(&gtp_genl_family);
> +	err = register_pernet_subsys(&gtp_net_ops);
>  	if (err < 0)
>  		goto unreg_rtnl_link;
>  
> -	err = register_pernet_subsys(&gtp_net_ops);
> +	err = genl_register_family(&gtp_genl_family);
>  	if (err < 0)
> -		goto unreg_genl_family;
> +		goto unreg_pernet_subsys;
>  
>  	pr_info("GTP module loaded (pdp ctx size %zd bytes)\n",
>  		sizeof(struct pdp_ctx));
>  	return 0;
>  
> -unreg_genl_family:
> -	genl_unregister_family(&gtp_genl_family);
> +unreg_pernet_subsys:
> +	unregister_pernet_subsys(&gtp_net_ops);
>  unreg_rtnl_link:
>  	rtnl_link_unregister(&gtp_link_ops);
>  error_out:
> -- 
> 2.33.8
>