Hi all,
This series contains two patches that are already available upstream:
- The first commit fixes a use-after-free[1], but introduced a null-ptr-deref[2]. - The second commit fixes it. [3]
I suggested waiting for both of them to be merged upstream and then applying them togheter to stable[4].
It should be applied to: - 6.13.y - 6.12.y - 6.6.y
I will send another series for - 6.1.y - 5.15.y - 5.10.y
because of conflicts.
[1]https://lore.kernel.org/all/20250128-vsock-transport-vs-autobind-v3-0-1cf570... [2]https://lore.kernel.org/all/67a09300.050a0220.d7c5a.008b.GAE@google.com/ [3]https://lore.kernel.org/all/20250210-vsock-linger-nullderef-v3-0-ef6244d02b5... [4]https://lore.kernel.org/all/2025020644-unwitting-scary-3c0d@gregkh/
Thanks, Luigi
--- Michal Luczaj (2): vsock: Keep the binding until socket destruction vsock: Orphan socket after transport release
net/vmw_vsock/af_vsock.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) --- base-commit: a1856aaa2ca74c88751f7d255dfa0c8c50fcc1ca change-id: 20250214-linux-rolling-stable-d73f0bed815d
Best regards,
From: Michal Luczaj mhal@rbox.co
commit fcdd2242c0231032fc84e1404315c245ae56322a upstream.
Preserve sockets bindings; this includes both resulting from an explicit bind() and those implicitly bound through autobind during connect().
Prevents socket unbinding during a transport reassignment, which fixes a use-after-free:
1. vsock_create() (refcnt=1) calls vsock_insert_unbound() (refcnt=2) 2. transport->release() calls vsock_remove_bound() without checking if sk was bound and moved to bound list (refcnt=1) 3. vsock_bind() assumes sk is in unbound list and before __vsock_insert_bound(vsock_bound_sockets()) calls __vsock_remove_bound() which does: list_del_init(&vsk->bound_table); // nop sock_put(&vsk->sk); // refcnt=0
BUG: KASAN: slab-use-after-free in __vsock_bind+0x62e/0x730 Read of size 4 at addr ffff88816b46a74c by task a.out/2057 dump_stack_lvl+0x68/0x90 print_report+0x174/0x4f6 kasan_report+0xb9/0x190 __vsock_bind+0x62e/0x730 vsock_bind+0x97/0xe0 __sys_bind+0x154/0x1f0 __x64_sys_bind+0x6e/0xb0 do_syscall_64+0x93/0x1b0 entry_SYSCALL_64_after_hwframe+0x76/0x7e
Allocated by task 2057: kasan_save_stack+0x1e/0x40 kasan_save_track+0x10/0x30 __kasan_slab_alloc+0x85/0x90 kmem_cache_alloc_noprof+0x131/0x450 sk_prot_alloc+0x5b/0x220 sk_alloc+0x2c/0x870 __vsock_create.constprop.0+0x2e/0xb60 vsock_create+0xe4/0x420 __sock_create+0x241/0x650 __sys_socket+0xf2/0x1a0 __x64_sys_socket+0x6e/0xb0 do_syscall_64+0x93/0x1b0 entry_SYSCALL_64_after_hwframe+0x76/0x7e
Freed by task 2057: kasan_save_stack+0x1e/0x40 kasan_save_track+0x10/0x30 kasan_save_free_info+0x37/0x60 __kasan_slab_free+0x4b/0x70 kmem_cache_free+0x1a1/0x590 __sk_destruct+0x388/0x5a0 __vsock_bind+0x5e1/0x730 vsock_bind+0x97/0xe0 __sys_bind+0x154/0x1f0 __x64_sys_bind+0x6e/0xb0 do_syscall_64+0x93/0x1b0 entry_SYSCALL_64_after_hwframe+0x76/0x7e
refcount_t: addition on 0; use-after-free. WARNING: CPU: 7 PID: 2057 at lib/refcount.c:25 refcount_warn_saturate+0xce/0x150 RIP: 0010:refcount_warn_saturate+0xce/0x150 __vsock_bind+0x66d/0x730 vsock_bind+0x97/0xe0 __sys_bind+0x154/0x1f0 __x64_sys_bind+0x6e/0xb0 do_syscall_64+0x93/0x1b0 entry_SYSCALL_64_after_hwframe+0x76/0x7e
refcount_t: underflow; use-after-free. WARNING: CPU: 7 PID: 2057 at lib/refcount.c:28 refcount_warn_saturate+0xee/0x150 RIP: 0010:refcount_warn_saturate+0xee/0x150 vsock_remove_bound+0x187/0x1e0 __vsock_release+0x383/0x4a0 vsock_release+0x90/0x120 __sock_release+0xa3/0x250 sock_close+0x14/0x20 __fput+0x359/0xa80 task_work_run+0x107/0x1d0 do_exit+0x847/0x2560 do_group_exit+0xb8/0x250 __x64_sys_exit_group+0x3a/0x50 x64_sys_call+0xfec/0x14f0 do_syscall_64+0x93/0x1b0 entry_SYSCALL_64_after_hwframe+0x76/0x7e
Fixes: c0cfa2d8a788 ("vsock: add multi-transports support") Reviewed-by: Stefano Garzarella sgarzare@redhat.com Signed-off-by: Michal Luczaj mhal@rbox.co Link: https://patch.msgid.link/20250128-vsock-transport-vs-autobind-v3-1-1cf57065b... Signed-off-by: Jakub Kicinski kuba@kernel.org --- net/vmw_vsock/af_vsock.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index f5d116a1bdea1a0b1a80488a27ce71ee636a65fa..ec4c1fbbcec7418d2e715bad30845cd95a9b270f 100644 --- a/net/vmw_vsock/af_vsock.c +++ b/net/vmw_vsock/af_vsock.c @@ -337,7 +337,10 @@ EXPORT_SYMBOL_GPL(vsock_find_connected_socket);
void vsock_remove_sock(struct vsock_sock *vsk) { - vsock_remove_bound(vsk); + /* Transport reassignment must not remove the binding. */ + if (sock_flag(sk_vsock(vsk), SOCK_DEAD)) + vsock_remove_bound(vsk); + vsock_remove_connected(vsk); } EXPORT_SYMBOL_GPL(vsock_remove_sock); @@ -821,12 +824,13 @@ static void __vsock_release(struct sock *sk, int level) */ lock_sock_nested(sk, level);
+ sock_orphan(sk); + if (vsk->transport) vsk->transport->release(vsk); else if (sock_type_connectible(sk->sk_type)) vsock_remove_sock(vsk);
- sock_orphan(sk); sk->sk_shutdown = SHUTDOWN_MASK;
skb_queue_purge(&sk->sk_receive_queue);
From: Michal Luczaj mhal@rbox.co
commit 78dafe1cf3afa02ed71084b350713b07e72a18fb upstream.
During socket release, sock_orphan() is called without considering that it sets sk->sk_wq to NULL. Later, if SO_LINGER is enabled, this leads to a null pointer dereferenced in virtio_transport_wait_close().
Orphan the socket only after transport release.
Partially reverts the 'Fixes:' commit.
KASAN: null-ptr-deref in range [0x0000000000000018-0x000000000000001f] lock_acquire+0x19e/0x500 _raw_spin_lock_irqsave+0x47/0x70 add_wait_queue+0x46/0x230 virtio_transport_release+0x4e7/0x7f0 __vsock_release+0xfd/0x490 vsock_release+0x90/0x120 __sock_release+0xa3/0x250 sock_close+0x14/0x20 __fput+0x35e/0xa90 __x64_sys_close+0x78/0xd0 do_syscall_64+0x93/0x1b0 entry_SYSCALL_64_after_hwframe+0x76/0x7e
Reported-by: syzbot+9d55b199192a4be7d02c@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=9d55b199192a4be7d02c Fixes: fcdd2242c023 ("vsock: Keep the binding until socket destruction") Tested-by: Luigi Leonardi leonardi@redhat.com Reviewed-by: Luigi Leonardi leonardi@redhat.com Signed-off-by: Michal Luczaj mhal@rbox.co Link: https://patch.msgid.link/20250210-vsock-linger-nullderef-v3-1-ef6244d02b54@r... Signed-off-by: Jakub Kicinski kuba@kernel.org --- net/vmw_vsock/af_vsock.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index ec4c1fbbcec7418d2e715bad30845cd95a9b270f..37299a7ca1876e58ff516b5112d44b171cb896b0 100644 --- a/net/vmw_vsock/af_vsock.c +++ b/net/vmw_vsock/af_vsock.c @@ -824,13 +824,19 @@ static void __vsock_release(struct sock *sk, int level) */ lock_sock_nested(sk, level);
- sock_orphan(sk); + /* Indicate to vsock_remove_sock() that the socket is being released and + * can be removed from the bound_table. Unlike transport reassignment + * case, where the socket must remain bound despite vsock_remove_sock() + * being called from the transport release() callback. + */ + sock_set_flag(sk, SOCK_DEAD);
if (vsk->transport) vsk->transport->release(vsk); else if (sock_type_connectible(sk->sk_type)) vsock_remove_sock(vsk);
+ sock_orphan(sk); sk->sk_shutdown = SHUTDOWN_MASK;
skb_queue_purge(&sk->sk_receive_queue);
On Fri, Feb 14, 2025 at 06:53:54PM +0100, Luigi Leonardi wrote:
Hi all,
This series contains two patches that are already available upstream:
- The first commit fixes a use-after-free[1], but introduced a
null-ptr-deref[2].
- The second commit fixes it. [3]
I suggested waiting for both of them to be merged upstream and then applying them togheter to stable[4].
It should be applied to:
- 6.13.y
- 6.12.y
- 6.6.y
I will send another series for
- 6.1.y
- 5.15.y
- 5.10.y
because of conflicts.
[1]https://lore.kernel.org/all/20250128-vsock-transport-vs-autobind-v3-0-1cf570... [2]https://lore.kernel.org/all/67a09300.050a0220.d7c5a.008b.GAE@google.com/ [3]https://lore.kernel.org/all/20250210-vsock-linger-nullderef-v3-0-ef6244d02b5... [4]https://lore.kernel.org/all/2025020644-unwitting-scary-3c0d@gregkh/
Thanks, Luigi
Michal Luczaj (2): vsock: Keep the binding until socket destruction vsock: Orphan socket after transport release
net/vmw_vsock/af_vsock.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
base-commit: a1856aaa2ca74c88751f7d255dfa0c8c50fcc1ca change-id: 20250214-linux-rolling-stable-d73f0bed815d
Best regards, -- Luigi Leonardi leonardi@redhat.com
Looks like I forgot to add my SoB to the commits, my bad.
For all the other stable trees (6.1, 5.15 and 5.10), there are some conflicts due to some indentation changes introduced by 135ffc7 ("bpf, vsock: Invoke proto::close on close()"). Should I backport this commit too? There is no real dependency on the commit in the Fixes tag ("vsock: support sockmap"). IMHO, this would help future backports, because of indentation conficts! Otherwise I can simply fix the patches. WDYT?
Cheers, Luigi
On 2/17/25 12:18, Luigi Leonardi wrote:
On Fri, Feb 14, 2025 at 06:53:54PM +0100, Luigi Leonardi wrote:
Hi all,
This series contains two patches that are already available upstream:
- The first commit fixes a use-after-free[1], but introduced a
null-ptr-deref[2].
- The second commit fixes it. [3]
I suggested waiting for both of them to be merged upstream and then applying them togheter to stable[4].
It should be applied to:
- 6.13.y
- 6.12.y
- 6.6.y
I will send another series for
- 6.1.y
- 5.15.y
- 5.10.y
because of conflicts.
[1]https://lore.kernel.org/all/20250128-vsock-transport-vs-autobind-v3-0-1cf570... [2]https://lore.kernel.org/all/67a09300.050a0220.d7c5a.008b.GAE@google.com/ [3]https://lore.kernel.org/all/20250210-vsock-linger-nullderef-v3-0-ef6244d02b5... [4]https://lore.kernel.org/all/2025020644-unwitting-scary-3c0d@gregkh/
Thanks, Luigi
Michal Luczaj (2): vsock: Keep the binding until socket destruction vsock: Orphan socket after transport release
net/vmw_vsock/af_vsock.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
base-commit: a1856aaa2ca74c88751f7d255dfa0c8c50fcc1ca change-id: 20250214-linux-rolling-stable-d73f0bed815d
Best regards, -- Luigi Leonardi leonardi@redhat.com
Looks like I forgot to add my SoB to the commits, my bad.
For all the other stable trees (6.1, 5.15 and 5.10), there are some conflicts due to some indentation changes introduced by 135ffc7 ("bpf, vsock: Invoke proto::close on close()"). Should I backport this commit too? There is no real dependency on the commit in the Fixes tag ("vsock: support sockmap"). IMHO, this would help future backports, because of indentation conficts! Otherwise I can simply fix the patches. WDYT?
Just a note: since sockmap does not support AF_VSOCK in those kernels <= 6.1, backporting 135ffc7 would introduce a (no-op) callback function vsock_close(), which would then be (unnecessarily) called on every vsock_release().
On Mon, Feb 17, 2025 at 08:45:57PM +0100, Michal Luczaj wrote:
On 2/17/25 12:18, Luigi Leonardi wrote:
On Fri, Feb 14, 2025 at 06:53:54PM +0100, Luigi Leonardi wrote:
Hi all,
This series contains two patches that are already available upstream:
- The first commit fixes a use-after-free[1], but introduced a
null-ptr-deref[2].
- The second commit fixes it. [3]
I suggested waiting for both of them to be merged upstream and then applying them togheter to stable[4].
It should be applied to:
- 6.13.y
- 6.12.y
- 6.6.y
I will send another series for
- 6.1.y
- 5.15.y
- 5.10.y
because of conflicts.
[1]https://lore.kernel.org/all/20250128-vsock-transport-vs-autobind-v3-0-1cf570... [2]https://lore.kernel.org/all/67a09300.050a0220.d7c5a.008b.GAE@google.com/ [3]https://lore.kernel.org/all/20250210-vsock-linger-nullderef-v3-0-ef6244d02b5... [4]https://lore.kernel.org/all/2025020644-unwitting-scary-3c0d@gregkh/
Thanks, Luigi
Michal Luczaj (2): vsock: Keep the binding until socket destruction vsock: Orphan socket after transport release
net/vmw_vsock/af_vsock.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
base-commit: a1856aaa2ca74c88751f7d255dfa0c8c50fcc1ca change-id: 20250214-linux-rolling-stable-d73f0bed815d
Best regards, -- Luigi Leonardi leonardi@redhat.com
Looks like I forgot to add my SoB to the commits, my bad.
For all the other stable trees (6.1, 5.15 and 5.10), there are some conflicts due to some indentation changes introduced by 135ffc7 ("bpf, vsock: Invoke proto::close on close()"). Should I backport this commit too? There is no real dependency on the commit in the Fixes tag ("vsock: support sockmap"). IMHO, this would help future backports, because of indentation conficts! Otherwise I can simply fix the patches. WDYT?
Just a note: since sockmap does not support AF_VSOCK in those kernels <= 6.1, backporting 135ffc7 would introduce a (no-op) callback function vsock_close(), which would then be (unnecessarily) called on every vsock_release().
But this is the same behavior we have now upstream (without considering sockmap), right?
Do you see any potential problems?
Thanks, Stefano
On 2/18/25 09:35, Stefano Garzarella wrote:
On Mon, Feb 17, 2025 at 08:45:57PM +0100, Michal Luczaj wrote:
On 2/17/25 12:18, Luigi Leonardi wrote:
On Fri, Feb 14, 2025 at 06:53:54PM +0100, Luigi Leonardi wrote:
Hi all,
This series contains two patches that are already available upstream:
- The first commit fixes a use-after-free[1], but introduced a
null-ptr-deref[2].
- The second commit fixes it. [3]
I suggested waiting for both of them to be merged upstream and then applying them togheter to stable[4].
It should be applied to:
- 6.13.y
- 6.12.y
- 6.6.y
I will send another series for
- 6.1.y
- 5.15.y
- 5.10.y
because of conflicts.
[1]https://lore.kernel.org/all/20250128-vsock-transport-vs-autobind-v3-0-1cf570... [2]https://lore.kernel.org/all/67a09300.050a0220.d7c5a.008b.GAE@google.com/ [3]https://lore.kernel.org/all/20250210-vsock-linger-nullderef-v3-0-ef6244d02b5... [4]https://lore.kernel.org/all/2025020644-unwitting-scary-3c0d@gregkh/
Thanks, Luigi
Michal Luczaj (2): vsock: Keep the binding until socket destruction vsock: Orphan socket after transport release
net/vmw_vsock/af_vsock.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
base-commit: a1856aaa2ca74c88751f7d255dfa0c8c50fcc1ca change-id: 20250214-linux-rolling-stable-d73f0bed815d
Best regards, -- Luigi Leonardi leonardi@redhat.com
Looks like I forgot to add my SoB to the commits, my bad.
For all the other stable trees (6.1, 5.15 and 5.10), there are some conflicts due to some indentation changes introduced by 135ffc7 ("bpf, vsock: Invoke proto::close on close()"). Should I backport this commit too? There is no real dependency on the commit in the Fixes tag ("vsock: support sockmap"). IMHO, this would help future backports, because of indentation conficts! Otherwise I can simply fix the patches. WDYT?
Just a note: since sockmap does not support AF_VSOCK in those kernels <= 6.1, backporting 135ffc7 would introduce a (no-op) callback function vsock_close(), which would then be (unnecessarily) called on every vsock_release().
But this is the same behavior we have now upstream (without considering sockmap), right?
Oh, right, that's true.
Do you see any potential problems?
No, nothing I can think of.
Note however that the comment above vsock_close() ("Dummy callback required by sockmap. See unconditional call of saved_close() in sock_map_close().") becomes somewhat misleading :)
On Tue, Feb 18, 2025 at 03:05:15PM +0100, Michal Luczaj wrote:
On 2/18/25 09:35, Stefano Garzarella wrote:
On Mon, Feb 17, 2025 at 08:45:57PM +0100, Michal Luczaj wrote:
On 2/17/25 12:18, Luigi Leonardi wrote:
On Fri, Feb 14, 2025 at 06:53:54PM +0100, Luigi Leonardi wrote:
Hi all,
This series contains two patches that are already available upstream:
- The first commit fixes a use-after-free[1], but introduced a
null-ptr-deref[2].
- The second commit fixes it. [3]
I suggested waiting for both of them to be merged upstream and then applying them togheter to stable[4].
It should be applied to:
- 6.13.y
- 6.12.y
- 6.6.y
I will send another series for
- 6.1.y
- 5.15.y
- 5.10.y
because of conflicts.
[1]https://lore.kernel.org/all/20250128-vsock-transport-vs-autobind-v3-0-1cf570... [2]https://lore.kernel.org/all/67a09300.050a0220.d7c5a.008b.GAE@google.com/ [3]https://lore.kernel.org/all/20250210-vsock-linger-nullderef-v3-0-ef6244d02b5... [4]https://lore.kernel.org/all/2025020644-unwitting-scary-3c0d@gregkh/
Thanks, Luigi
Michal Luczaj (2): vsock: Keep the binding until socket destruction vsock: Orphan socket after transport release
net/vmw_vsock/af_vsock.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
base-commit: a1856aaa2ca74c88751f7d255dfa0c8c50fcc1ca change-id: 20250214-linux-rolling-stable-d73f0bed815d
Best regards, -- Luigi Leonardi leonardi@redhat.com
Looks like I forgot to add my SoB to the commits, my bad.
For all the other stable trees (6.1, 5.15 and 5.10), there are some conflicts due to some indentation changes introduced by 135ffc7 ("bpf, vsock: Invoke proto::close on close()"). Should I backport this commit too? There is no real dependency on the commit in the Fixes tag ("vsock: support sockmap"). IMHO, this would help future backports, because of indentation conficts! Otherwise I can simply fix the patches. WDYT?
Just a note: since sockmap does not support AF_VSOCK in those kernels <= 6.1, backporting 135ffc7 would introduce a (no-op) callback function vsock_close(), which would then be (unnecessarily) called on every vsock_release().
But this is the same behavior we have now upstream (without considering sockmap), right?
Oh, right, that's true.
Do you see any potential problems?
No, nothing I can think of.
Note however that the comment above vsock_close() ("Dummy callback required by sockmap. See unconditional call of saved_close() in sock_map_close().") becomes somewhat misleading :)
Yeah, we can mention in the commit description of the backport that we backport it just to reduce conflicts but sockmap features are not backported. I'd touch as less as possibile in the patch, otherwise IMHO is better to just fix the conflicts in the 2 patches.
Thanks, Stefano
linux-stable-mirror@lists.linaro.org
-
Luigi Leonardi -
Michal Luczaj -
Stefano Garzarella