kobject: fix uevent socket use-after-free on net namespace delete
Subsystems (3)
PATCH 1/1 kobject: fix uevent socket use-after-free on net namespace delete
When a network namespace is deleted, we free its uevent socket. But sometimes a device in that namespace is still being removed and tries to send a uevent through that socket. The socket is already freed, so KASAN reports a use-after-free.
This patch attempts the fix of taking a lock around both the send path and the free path, and clear the pointer before freeing so nobody uses it after free.
--- lib/kobject_uevent.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)
lib/kobject_uevent.c +17 -7
@@ -411,9 +411,15 @@ static int kobject_uevent_net_broadcast(struct kobject *kobj, devpath); else { const struct net *net = container_of(ns, struct net, ns); +struct uevent_sock *ue_sk; - ret =uevent_net_broadcast_tagged (net->uevent_sock->sk, env, - action_string, devpath); +mutex_lock (&uevent_sock_mutex); + ue_sk = net->uevent_sock; +if (ue_sk && ue_sk->sk) + ret =uevent_net_broadcast_tagged (ue_sk->sk, env, + action_string, + devpath); +mutex_unlock (&uevent_sock_mutex); }#endif
@@ -804,13 +810,17 @@ static int uevent_net_init(struct net *net)static void uevent_net_exit (struct net *net) { -struct uevent_sock *ue_sk = net->uevent_sock; +struct uevent_sock *ue_sk; -if (sock_net (ue_sk->sk)->user_ns == &init_user_ns) { -mutex_lock (&uevent_sock_mutex); +mutex_lock (&uevent_sock_mutex); + ue_sk = net->uevent_sock; + net->uevent_sock =NULL ; +if (ue_sk &&sock_net (ue_sk->sk)->user_ns == &init_user_ns)list_del (&ue_sk->list); -mutex_unlock (&uevent_sock_mutex); - } +mutex_unlock (&uevent_sock_mutex); + +if (!ue_sk) +return ;netlink_kernel_release (ue_sk->sk);kfree (ue_sk);
Signature · 1 line
2.55.0
Greg KH
maintainerQuoted 9 lines · Adarsh Das
When a network namespace is deleted, we free its uevent socket. But sometimes a device in that namespace is still being removed and tries to send a uevent through that socket. The socket is already freed, so KASAN reports a use-after-free.
This patch attempts the fix of taking a lock around both the send path and the free path, and clear the pointer before freeing so nobody uses it after free.
Quoted 6 lines
Reported-by syzbot <[email protected]>Signed-off-by Adarsh Das <[email protected]>--- lib/kobject_uevent.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)
Did you forget an Assisted-by: tag?
How was this tested?
thanks,
greg k-h
Hi,
Sorry, I wasn't aware of the Assisted-by tag before. Should I send a V2 with it?
For testing I ran syzbot's C repro with -smp 4 on master, with multiple parallel instances of the repro. I was able to reproduce the bug:
BUG: KASAN: slab-use-after-free in kobject_uevent_env+
Read of size 8 at addr ffff88800fa7f090 by task kworker/u16:5
CPU: 3
Workqueue: ib-unreg-wq ib_unregister_work
Call Trace:
kasan_report
kobject_uevent_env+0xc71/0xd90
device_del+0x7c7/0xcf0
disable_device+0x1d2/0x260
__ib_unregister_device+0x26b/0x400
ib_unregister_work+0x19/0x30
worker_thread
kthread
Allocated by task 498:
uevent_net_init+0xe5/0x340
setup_net
copy_net_ns
unshare_nsproxy_namespaces
ksys_unshare
Freed by task 63:
kfree
ops_undo_list
cleanup_net
worker_thread
The buggy address belongs to the cache kmalloc-32 of size 32With this patch applied, I ran the same repro repeatedly (including parallel runs) and did not see the KASAN error anymore.
Greg KH
maintainerA: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
A: No.
Q: Should I include quotations after my reply?
http://daringfireball.net/2007/07/on_top
Quoted 4 lines · Adarsh Das
Hi,
Sorry, I wasn't aware of the Assisted-by tag before. Should I send a V2 with it?
Please do.
Quoted 30 lines
For testing I ran syzbot's C repro with -smp 4 on master, with multiple parallel instances of the repro. I was able to reproduce the bug:
BUG: KASAN: slab-use-after-free in kobject_uevent_env+ Read of size 8 at addr ffff88800fa7f090 by task kworker/u16:5 CPU: 3 Workqueue: ib-unreg-wq ib_unregister_work Call Trace: kasan_report kobject_uevent_env+0xc71/0xd90 device_del+0x7c7/0xcf0 disable_device+0x1d2/0x260 __ib_unregister_device+0x26b/0x400 ib_unregister_work+0x19/0x30 worker_thread kthread Allocated by task 498: uevent_net_init+0xe5/0x340 setup_net copy_net_ns unshare_nsproxy_namespaces ksys_unshare Freed by task 63: kfree ops_undo_list cleanup_net worker_thread The buggy address belongs to the cache kmalloc-32 of size 32
Quoted 2 lines
With this patch applied, I ran the same repro repeatedly (including parallel runs) and did not see the KASAN error anymore.
Did syzbot also test this?
And I have no context here at all, sorry, please realize that some of us get 1000+ emails a day...
thanks,
greg k-h
PATCH v2 1/1 kobject: fix uevent socket use-after-free on net namespace delete
In reply to Adarsh DasHi Greg,
v2 adds Assisted-by and Tested-by tags as requested.
Local testing: syzbot C repro on unfixed master with -smp 4 (parallel runs) reproduced the KASAN UAF. With patch applied, no KASAN.
Syzbot testing: #syz test with patch on upstream — Result OK
https://syzkaller.appspot.com/bug?extid=4393dfdddf166f2de2b0
Adarsh
PATCH v2 1/1 kobject: fix uevent socket use-after-free on net namespace delete
In reply to Adarsh DasWhen a network namespace is deleted, we free its uevent socket. But sometimes a device in that namespace is still being removed and tries to send a uevent through that socket. The socket is already freed, so KASAN reports a use-after-free.
This patch attempts the fix of taking a lock around both the send path and the free path, and clear the pointer before freeing so nobody uses it after free.
--- lib/kobject_uevent.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)
lib/kobject_uevent.c +17 -7
@@ -411,9 +411,15 @@ static int kobject_uevent_net_broadcast(struct kobject *kobj, devpath); else { const struct net *net = container_of(ns, struct net, ns); +struct uevent_sock *ue_sk; - ret =uevent_net_broadcast_tagged (net->uevent_sock->sk, env, - action_string, devpath); +mutex_lock (&uevent_sock_mutex); + ue_sk = net->uevent_sock; +if (ue_sk && ue_sk->sk) + ret =uevent_net_broadcast_tagged (ue_sk->sk, env, + action_string, + devpath); +mutex_unlock (&uevent_sock_mutex); }#endif
@@ -804,13 +810,17 @@ static int uevent_net_init(struct net *net)static void uevent_net_exit (struct net *net) { -struct uevent_sock *ue_sk = net->uevent_sock; +struct uevent_sock *ue_sk; -if (sock_net (ue_sk->sk)->user_ns == &init_user_ns) { -mutex_lock (&uevent_sock_mutex); +mutex_lock (&uevent_sock_mutex); + ue_sk = net->uevent_sock; + net->uevent_sock =NULL ; +if (ue_sk &&sock_net (ue_sk->sk)->user_ns == &init_user_ns)list_del (&ue_sk->list); -mutex_unlock (&uevent_sock_mutex); - } +mutex_unlock (&uevent_sock_mutex); + +if (!ue_sk) +return ;netlink_kernel_release (ue_sk->sk);kfree (ue_sk);
Signature · 1 line
2.55.0
Greg KH
maintainerQuoted 24 lines · Adarsh Das
When a network namespace is deleted, we free its uevent socket. But sometimes a device in that namespace is still being removed and tries to send a uevent through that socket. The socket is already freed, so KASAN reports a use-after-free.
This patch attempts the fix of taking a lock around both the send path and the free path, and clear the pointer before freeing so nobody uses it after free.
Reported-by syzbot <[email protected]>Tested-by syzbot <[email protected]>Tested-by Adarsh Das <[email protected]>Assisted-by Cursor AISigned-off-by Adarsh Das <[email protected]>--- lib/kobject_uevent.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)lib/kobject_uevent.c +0
-0
lib/kobject_uevent.c static int kobject_uevent_net_broadcast(struct kobject *kobj, · line 416
Quoted 8 lines
lib/kobject_uevent.c +2
-2@@ -411,9 +411,15 @@ static int kobject_uevent_net_broadcast(struct kobject *kobj, devpath); else { const struct net *net = container_of(ns, struct net, ns); +struct uevent_sock *ue_sk; - ret =uevent_net_broadcast_tagged (net->uevent_sock->sk, env, - action_string, devpath); +mutex_lock (&uevent_sock_mutex);
Can't you use a guard to make this a lot cleaner?
thanks,
greg k-h
PATCH v3 1/1 kobject: fix uevent socket use-after-free on net namespace delete
When a network namespace is deleted, we free its uevent socket. But sometimes a device in that namespace is still being removed and tries to send a uevent through that socket. The socket is already freed, so KASAN reports a use-after-free.
This patch attempts the fix of taking a lock around both the send path and the free path, and clear the pointer before freeing so nobody uses it after free.
--- lib/kobject_uevent.c | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-)
lib/kobject_uevent.c +19 -8
@@ -13,6 +13,7 @@* Greg Kroah-Hartman <[email protected]> */ +#include <linux/cleanup.h> #include <linux/spinlock.h> #include <linux/string.h> #include <linux/kobject.h>
@@ -411,9 +412,14 @@ static int kobject_uevent_net_broadcast(struct kobject *kobj, devpath); else { const struct net *net = container_of(ns, struct net, ns); - - ret =uevent_net_broadcast_tagged (net->uevent_sock->sk, env, - action_string, devpath); +struct uevent_sock *ue_sk; + +guard (mutex)(&uevent_sock_mutex); + ue_sk = net->uevent_sock; +if (ue_sk && ue_sk->sk) + ret =uevent_net_broadcast_tagged (ue_sk->sk, env, + action_string, + devpath); }#endif
@@ -804,14 +810,19 @@ static int uevent_net_init(struct net *net)static void uevent_net_exit (struct net *net) { -struct uevent_sock *ue_sk = net->uevent_sock; +struct uevent_sock *ue_sk; -if (sock_net (ue_sk->sk)->user_ns == &init_user_ns) { -mutex_lock (&uevent_sock_mutex); -list_del (&ue_sk->list); -mutex_unlock (&uevent_sock_mutex); + { +guard (mutex)(&uevent_sock_mutex); + ue_sk = net->uevent_sock; + net->uevent_sock =NULL ; +if (ue_sk &&sock_net (ue_sk->sk)->user_ns == &init_user_ns) +list_del (&ue_sk->list); } +if (!ue_sk) +return ; +netlink_kernel_release (ue_sk->sk);kfree (ue_sk); }
Signature · 1 line
2.55.0
Greg KH
maintainerQuoted 77 lines · Adarsh Das
When a network namespace is deleted, we free its uevent socket. But sometimes a device in that namespace is still being removed and tries to send a uevent through that socket. The socket is already freed, so KASAN reports a use-after-free.
This patch attempts the fix of taking a lock around both the send path and the free path, and clear the pointer before freeing so nobody uses it after free.
Reported-by syzbot <[email protected]>Tested-by syzbot <[email protected]>Tested-by Adarsh Das <[email protected]>Assisted-by LLMSigned-off-by Adarsh Das <[email protected]>--- lib/kobject_uevent.c | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-)lib/kobject_uevent.c +19
-8@@ -13,6 +13,7 @@* Greg Kroah-Hartman <[email protected]> */ +#include <linux/cleanup.h> #include <linux/spinlock.h> #include <linux/string.h> #include <linux/kobject.h> @@ -411,9 +412,14 @@ static int kobject_uevent_net_broadcast(struct kobject *kobj, devpath); else { const struct net *net = container_of(ns, struct net, ns); - - ret =uevent_net_broadcast_tagged (net->uevent_sock->sk, env, - action_string, devpath); +struct uevent_sock *ue_sk; + +guard (mutex)(&uevent_sock_mutex); + ue_sk = net->uevent_sock; +if (ue_sk && ue_sk->sk) + ret =uevent_net_broadcast_tagged (ue_sk->sk, env, + action_string, + devpath); }#endif @@ -804,14 +810,19 @@ static int uevent_net_init(struct net *net)static void uevent_net_exit (struct net *net) { -struct uevent_sock *ue_sk = net->uevent_sock; +struct uevent_sock *ue_sk; -if (sock_net (ue_sk->sk)->user_ns == &init_user_ns) { -mutex_lock (&uevent_sock_mutex); -list_del (&ue_sk->list); -mutex_unlock (&uevent_sock_mutex); + { +guard (mutex)(&uevent_sock_mutex); + ue_sk = net->uevent_sock; + net->uevent_sock =NULL ; +if (ue_sk &&sock_net (ue_sk->sk)->user_ns == &init_user_ns) +list_del (&ue_sk->list); } +if (!ue_sk) +return ; +netlink_kernel_release (ue_sk->sk);kfree (ue_sk); }Signature · 1 line
2.55.0
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree.
You are receiving this message because of the following common error(s) as indicated below:
- This looks like a new version of a previously submitted patch, but you did not list below the --- line any changes from the previous version. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/process/submitting-patches.rst for what needs to be done here to properly describe this.
If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers.
thanks,
greg k-h's patch email bot
Greg KH
maintainerlib/kobject_uevent.c static int uevent_net_init(struct net *net) · line 816
Quoted 13 lines · Adarsh Das
lib/kobject_uevent.c +3
-5@@ -804,14 +810,19 @@ static int uevent_net_init(struct net *net)static void uevent_net_exit (struct net *net) { -struct uevent_sock *ue_sk = net->uevent_sock; +struct uevent_sock *ue_sk; -if (sock_net (ue_sk->sk)->user_ns == &init_user_ns) { -mutex_lock (&uevent_sock_mutex); -list_del (&ue_sk->list); -mutex_unlock (&uevent_sock_mutex); + { +guard (mutex)(&uevent_sock_mutex);
LLMs don't know how to use these correctly, shouldn't this just be a scoped_guard()? No extra {} like that please, it's not normal kernel coding style at all.
thanks,
greg k-h
PATCH v4 1/1 kobject: fix uevent socket use-after-free on net namespace delete
When a network namespace is deleted, we free its uevent socket. But sometimes a device in that namespace is still being removed and tries to send a uevent through that socket. The socket is already freed, so KASAN reports a use-after-free.
Take a lock around both the send path and the free path, and clear the pointer before freeing so nobody uses it after free.
---
v4:
- Use scoped_guard(mutex) instead of block in uevent_net_exit
v3:
- Use guard(mutex) per review
v2:
- Add Assisted-by and Tested-by tags per review
lib/kobject_uevent.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-)
lib/kobject_uevent.c +18 -8
@@ -13,6 +13,7 @@* Greg Kroah-Hartman <[email protected]> */ +#include <linux/cleanup.h> #include <linux/spinlock.h> #include <linux/string.h> #include <linux/kobject.h>
@@ -411,9 +412,14 @@ static int kobject_uevent_net_broadcast(struct kobject *kobj, devpath); else { const struct net *net = container_of(ns, struct net, ns); - - ret =uevent_net_broadcast_tagged (net->uevent_sock->sk, env, - action_string, devpath); +struct uevent_sock *ue_sk; + +guard (mutex)(&uevent_sock_mutex); + ue_sk = net->uevent_sock; +if (ue_sk && ue_sk->sk) + ret =uevent_net_broadcast_tagged (ue_sk->sk, env, + action_string, + devpath); }#endif
@@ -804,14 +810,18 @@ static int uevent_net_init(struct net *net)static void uevent_net_exit (struct net *net) { -struct uevent_sock *ue_sk = net->uevent_sock; +struct uevent_sock *ue_sk; -if (sock_net (ue_sk->sk)->user_ns == &init_user_ns) { -mutex_lock (&uevent_sock_mutex); -list_del (&ue_sk->list); -mutex_unlock (&uevent_sock_mutex); + scoped_guard(mutex, &uevent_sock_mutex) { + ue_sk = net->uevent_sock; + net->uevent_sock =NULL ; +if (ue_sk &&sock_net (ue_sk->sk)->user_ns == &init_user_ns) +list_del (&ue_sk->list); } +if (!ue_sk) +return ; +netlink_kernel_release (ue_sk->sk);kfree (ue_sk); }
Signature · 1 line
2.55.0