[PATCH bpf] xsk: fix init race causing NPD/UAF in xsk_create()

[PATCH bpf] xsk: fix init race causing NPD/UAF in xsk_create()

[Kery Qi]

xsk_init() registers the PF_XDP socket family before xsk_net_ops.
This exposes .create = xsk_create() to user space while per-netns
state (net->xdp.lock/list) is still uninitialized.

A task with CAP_NET_RAW can trigger this during boot/module load by
calling socket(PF_XDP/AF_XDP, SOCK_RAW, 0) concurrently with xsk_init(),
leading to NULL deref, list/lock corruption or use-after-free.

Register the pernet subsystem (and prerequisites) first and only then
sock_register() PF_XDP. Update the error unwind to match the new order.

Similar to CVE-2024-26793 (GTP init ordering bug).

Fixes: c0c77d8fb787 ("xsk: add user memory registration support sockopt")
Signed-off-by: Kery Qi <qikeyu2017@gmail.com>
---
 net/xdp/xsk.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index f093c3453f64..63b48e4b8b65 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -1922,10 +1922,6 @@ static int __init xsk_init(void)
 	if (err)
 		goto out;
 
-	err = sock_register(&xsk_family_ops);
-	if (err)
-		goto out_proto;
-
 	err = register_pernet_subsys(&xsk_net_ops);
 	if (err)
 		goto out_sk;
@@ -1942,16 +1938,20 @@ static int __init xsk_init(void)
 		goto out_unreg_notif;
 	}
 
+	err = sock_register(&xsk_family_ops);
+	if (err)
+		goto out_proto;
+
 	return 0;
 
 out_unreg_notif:
 	unregister_netdevice_notifier(&xsk_netdev_notifier);
 out_pernet:
 	unregister_pernet_subsys(&xsk_net_ops);
-out_sk:
-	sock_unregister(PF_XDP);
 out_proto:
 	proto_unregister(&xsk_proto);
+out_sk:
+	sock_unregister(PF_XDP);
 out:
 	return err;
 }
-- 
2.34.1

Re: [PATCH bpf] xsk: fix init race causing NPD/UAF in xsk_create()

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 1972 bytes --]

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index f093c3453f64..63b48e4b8b65 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -1922,10 +1922,6 @@ static int __init xsk_init(void)
>  	if (err)
>  		goto out;
>
> -	err = sock_register(&xsk_family_ops);
> -	if (err)
> -		goto out_proto;
> -
>  	err = register_pernet_subsys(&xsk_net_ops);
>  	if (err)
>  		goto out_sk;

[ ... ]

> @@ -1942,16 +1938,20 @@ static int __init xsk_init(void)
>  		goto out_unreg_notif;
>  	}
>
> +	err = sock_register(&xsk_family_ops);
> +	if (err)
> +		goto out_proto;
> +
>  	return 0;
>
>  out_unreg_notif:
>  	unregister_netdevice_notifier(&xsk_netdev_notifier);
>  out_pernet:
>  	unregister_pernet_subsys(&xsk_net_ops);
> -out_sk:
> -	sock_unregister(PF_XDP);
>  out_proto:
>  	proto_unregister(&xsk_proto);
> +out_sk:
> +	sock_unregister(PF_XDP);

The error unwind path looks incorrect. If register_pernet_subsys() fails,
the code jumps to out_sk which now calls sock_unregister(PF_XDP). However,
at that point sock_register() has not been called yet since it was moved
to after kmem_cache_create().

Looking at the execution path:

    proto_register() succeeds
    register_pernet_subsys() fails
    goto out_sk
    -> out_sk: sock_unregister(PF_XDP)  // sock_register() was never called

The same issue affects register_netdevice_notifier() and kmem_cache_create()
failures - they all eventually fall through to out_sk and call
sock_unregister() before sock_register() was ever called.

Should out_sk be placed before out_proto in the error unwind section, or
should the goto targets be updated? The original code had the error unwind
labels in the correct order relative to the registration sequence.

>  out:
>  	return err;
>  }


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/20814824025

[PATCH bpf] xsk: fix init race causing NPD/UAF in xsk_create()

[Kery Qi]

xsk_init() registers the PF_XDP socket family before xsk_net_ops.
This exposes .create = xsk_create() to user space while per-netns
state (net->xdp.lock/list) is still uninitialized.

A task with CAP_NET_RAW can trigger this during boot/module load by
calling socket(PF_XDP/AF_XDP, SOCK_RAW, 0) concurrently with xsk_init(),
leading to NULL deref, list/lock corruption or use-after-free.

Register the pernet subsystem (and prerequisites) first and only then
sock_register() PF_XDP. Update the error unwind to match the new order.

Similar to CVE-2024-26793 (GTP init ordering bug).

Fixes: c0c77d8fb787 ("xsk: add user memory registration support sockopt")
Signed-off-by: Kery Qi <qikeyu2017@gmail.com>
---
 net/xdp/xsk.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index f093c3453f64..63b48e4b8b65 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -1922,10 +1922,6 @@ static int __init xsk_init(void)
 	if (err)
 		goto out;
 
-	err = sock_register(&xsk_family_ops);
-	if (err)
-		goto out_proto;
-
 	err = register_pernet_subsys(&xsk_net_ops);
 	if (err)
 		goto out_sk;
@@ -1942,16 +1938,20 @@ static int __init xsk_init(void)
 		goto out_unreg_notif;
 	}
 
+	err = sock_register(&xsk_family_ops);
+	if (err)
+		goto out_proto;
+
 	return 0;
 
 out_unreg_notif:
 	unregister_netdevice_notifier(&xsk_netdev_notifier);
 out_pernet:
 	unregister_pernet_subsys(&xsk_net_ops);
-out_sk:
-	sock_unregister(PF_XDP);
 out_proto:
 	proto_unregister(&xsk_proto);
+out_sk:
+	sock_unregister(PF_XDP);
 out:
 	return err;
 }
-- 
2.34.1

Re: [PATCH bpf] xsk: fix init race causing NPD/UAF in xsk_create()

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 1364 bytes --]

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index f093c3453f64..63b48e4b8b65 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -1942,16 +1938,20 @@ static int __init xsk_init(void)
>  		goto out_unreg_notif;
>  	}
>
> +	err = sock_register(&xsk_family_ops);
> +	if (err)
> +		goto out_proto;

If sock_register() fails here, does xsk_tx_generic_cache get leaked?
The kmem_cache was successfully created just above this, but the error
path at out_proto does not appear to call kmem_cache_destroy().

Looking at the error unwind below:

> +
>  	return 0;
>
>  out_unreg_notif:
>  	unregister_netdevice_notifier(&xsk_netdev_notifier);
>  out_pernet:
>  	unregister_pernet_subsys(&xsk_net_ops);
> -out_sk:
> -	sock_unregister(PF_XDP);
>  out_proto:
>  	proto_unregister(&xsk_proto);

The path from sock_register() failure goes to out_proto, which only calls
proto_unregister() before falling through. There is no kmem_cache_destroy()
for xsk_tx_generic_cache in this error path.

Should there be a new label between out_unreg_notif and out_pernet that
destroys the kmem_cache, and should out_proto jump there instead?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/20815690741

[PATCH bpf] xsk: fix init race causing NPD/UAF in xsk_create()

[Kery Qi]

xsk_init() previously registered the PF_XDP socket family before the
per-net subsystem and other prerequisites (netdevice notifier, caches)
were fully initialized.

This exposed .create = xsk_create() to user space while per-netns
state (net->xdp.lock/list) was still uninitialized. A task with
CAP_NET_RAW could trigger this during boot/module load by calling
socket(PF_XDP, SOCK_RAW, 0) concurrently with xsk_init(), leading
to a NULL pointer dereference or use-after-free in the list manipulation.

To fix this, move sock_register() to the end of the initialization
sequence, ensuring that all required kernel structures are ready before
exposing the AF_XDP interface to userspace.

Accordingly, reorder the error unwind path to ensure proper cleanup
in reverse order of initialization. Also, explicitly add
kmem_cache_destroy() in the error path to prevent leaking
xsk_tx_generic_cache if the registration fails.

Fixes: c0c77d8fb787 ("xsk: add user memory registration support sockopt")
Signed-off-by: Kery Qi <qikeyu2017@gmail.com>
---
 net/xdp/xsk.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index f093c3453f64..58e9c61c29e0 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -23,6 +23,7 @@
 #include <linux/netdevice.h>
 #include <linux/rculist.h>
 #include <linux/vmalloc.h>
+#include <linux/slab.h>
 #include <net/xdp_sock_drv.h>
 #include <net/busy_poll.h>
 #include <net/netdev_lock.h>
@@ -1922,10 +1923,6 @@ static int __init xsk_init(void)
 	if (err)
 		goto out;
 
-	err = sock_register(&xsk_family_ops);
-	if (err)
-		goto out_proto;
-
 	err = register_pernet_subsys(&xsk_net_ops);
 	if (err)
 		goto out_sk;
@@ -1942,16 +1939,21 @@ static int __init xsk_init(void)
 		goto out_unreg_notif;
 	}
 
+	err = sock_register(&xsk_family_ops);
+	if (err)
+		goto out_proto;
+
 	return 0;
 
+out_proto:
+	proto_unregister(&xsk_proto);
+	kmem_cache_destroy(xsk_tx_generic_cache);
 out_unreg_notif:
 	unregister_netdevice_notifier(&xsk_netdev_notifier);
 out_pernet:
 	unregister_pernet_subsys(&xsk_net_ops);
 out_sk:
 	sock_unregister(PF_XDP);
-out_proto:
-	proto_unregister(&xsk_proto);
 out:
 	return err;
 }
-- 
2.34.1

Re: [PATCH bpf] xsk: fix init race causing NPD/UAF in xsk_create()

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 1902 bytes --]

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index f093c3453f64..58e9c61c29e0 100644

[ ... ]

> @@ -1922,10 +1923,6 @@ static int __init xsk_init(void)
>  	if (err)
>  		goto out;
>
> -	err = sock_register(&xsk_family_ops);
> -	if (err)
> -		goto out_proto;
> -
>  	err = register_pernet_subsys(&xsk_net_ops);
>  	if (err)
>  		goto out_sk;

[ ... ]

> @@ -1942,16 +1939,21 @@ static int __init xsk_init(void)
>  		goto out_unreg_notif;
>  	}
>
> +	err = sock_register(&xsk_family_ops);
> +	if (err)
> +		goto out_proto;
> +
>  	return 0;
>
> +out_proto:
> +	proto_unregister(&xsk_proto);
> +	kmem_cache_destroy(xsk_tx_generic_cache);
>  out_unreg_notif:
>  	unregister_netdevice_notifier(&xsk_netdev_notifier);
>  out_pernet:
>  	unregister_pernet_subsys(&xsk_net_ops);
>  out_sk:
>  	sock_unregister(PF_XDP);
                ^^^^^^^^^^^^^^^^

With sock_register() now at the end of xsk_init(), the out_sk label is
reached via goto from register_pernet_subsys(), register_netdevice_notifier(),
or kmem_cache_create() failures - all of which occur before sock_register()
is called.

Shouldn't out_sk be removed entirely, and the gotos redirected? For example,
if register_pernet_subsys() fails, we goto out_sk and call sock_unregister()
on a family that was never registered. The same applies to the other error
paths that fall through to out_sk.

While sock_unregister() on an unregistered family appears to be benign (it
just sets NULL to NULL), it will print "NET: Unregistered PF_XDP protocol
family" to the kernel log even though it was never registered in the first
place.

> -out_proto:
> -	proto_unregister(&xsk_proto);
>  out:
>  	return err;
>  }


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/20848468294

[PATCH bpf] xsk: fix init race causing NPD/UAF in xsk_create()

[Kery Qi]

xsk_init() previously registered the PF_XDP socket family before the
per-net subsystem and other prerequisites (netdevice notifier, caches)
were fully initialized.

This exposed .create = xsk_create() to user space while per-netns
state (net->xdp.lock/list) was still uninitialized. A task with
CAP_NET_RAW could trigger this during boot/module load by calling
socket(PF_XDP, SOCK_RAW, 0) concurrently with xsk_init(), leading
to a NULL pointer dereference or use-after-free in the list manipulation.

To fix this, move sock_register() to the end of the initialization
sequence, ensuring that all required kernel structures are ready before
exposing the AF_XDP interface to userspace.

Accordingly, reorder the error unwind path to ensure proper cleanup
in reverse order of initialization. Also, explicitly add
kmem_cache_destroy() in the error path to prevent leaking
xsk_tx_generic_cache if the registration fails.

Fixes: c0c77d8fb787 ("xsk: add user memory registration support sockopt")
Signed-off-by: Kery Qi <qikeyu2017@gmail.com>
---
 net/xdp/xsk.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index f093c3453f64..d402f23dfd8e 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -23,6 +23,7 @@
 #include <linux/netdevice.h>
 #include <linux/rculist.h>
 #include <linux/vmalloc.h>
+#include <linux/slab.h>
 #include <net/xdp_sock_drv.h>
 #include <net/busy_poll.h>
 #include <net/netdev_lock.h>
@@ -1922,13 +1923,9 @@ static int __init xsk_init(void)
 	if (err)
 		goto out;
 
-	err = sock_register(&xsk_family_ops);
-	if (err)
-		goto out_proto;
-
 	err = register_pernet_subsys(&xsk_net_ops);
 	if (err)
-		goto out_sk;
+		goto out_proto;
 
 	err = register_netdevice_notifier(&xsk_netdev_notifier);
 	if (err)
@@ -1939,17 +1936,21 @@ static int __init xsk_init(void)
 						 0, SLAB_HWCACHE_ALIGN, NULL);
 	if (!xsk_tx_generic_cache) {
 		err = -ENOMEM;
-		goto out_unreg_notif;
+		goto out_notifier;
 	}
 
+	err = sock_register(&xsk_family_ops);
+	if (err)
+		goto out_cache;
+
 	return 0;
 
-out_unreg_notif:
+out_cache:
+	kmem_cache_destroy(xsk_tx_generic_cache);
+out_notifier:
 	unregister_netdevice_notifier(&xsk_netdev_notifier);
 out_pernet:
 	unregister_pernet_subsys(&xsk_net_ops);
-out_sk:
-	sock_unregister(PF_XDP);
 out_proto:
 	proto_unregister(&xsk_proto);
 out:
-- 
2.34.1

Re: [PATCH bpf] xsk: fix init race causing NPD/UAF in xsk_create()

[Stanislav Fomichev]

On 01/09, Kery Qi wrote:
> xsk_init() previously registered the PF_XDP socket family before the
> per-net subsystem and other prerequisites (netdevice notifier, caches)
> were fully initialized.
> 
> This exposed .create = xsk_create() to user space while per-netns
> state (net->xdp.lock/list) was still uninitialized. A task with
> CAP_NET_RAW could trigger this during boot/module load by calling
> socket(PF_XDP, SOCK_RAW, 0) concurrently with xsk_init(), leading
> to a NULL pointer dereference or use-after-free in the list manipulation.
> 
> To fix this, move sock_register() to the end of the initialization
> sequence, ensuring that all required kernel structures are ready before
> exposing the AF_XDP interface to userspace.
> 
> Accordingly, reorder the error unwind path to ensure proper cleanup
> in reverse order of initialization. Also, explicitly add
> kmem_cache_destroy() in the error path to prevent leaking
> xsk_tx_generic_cache if the registration fails.

Is it something that you've hit in real life? xsk_init happens
so early during the init process (fs_init) that I don't understand
why the oder would matter.