netdev - Re: [PATCH net 1/2] vsock: Orphan socket after transport release

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: <jfkqsbbq5um6nmlhnxxgx3eg7aopnwaddqvcj7s6svmpujswub@42sq2pawnsxn>
Date: Tue, 4 Feb 2025 16:44:13 +0100
From: Luigi Leonardi <leonardi@...hat.com>
To: Stefano Garzarella <sgarzare@...hat.com>
Cc: Michal Luczaj <mhal@...x.co>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, netdev@...r.kernel.org, 
	syzbot+9d55b199192a4be7d02c@...kaller.appspotmail.com, fstornio@...hat.com
Subject: Re: [PATCH net 1/2] vsock: Orphan socket after transport release

On Tue, Feb 04, 2025 at 11:32:54AM +0100, Stefano Garzarella wrote:
>On Tue, Feb 04, 2025 at 01:29:52AM +0100, Michal Luczaj wrote:
>>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@...kaller.appspotmail.com
>>Closes: https://syzkaller.appspot.com/bug?extid=9d55b199192a4be7d02c
>>Fixes: fcdd2242c023 ("vsock: Keep the binding until socket destruction")
>
>Looking better at that patch, can you check if we break commit
>3a5cc90a4d17 ("vsock/virtio: remove socket from connected/bound list 
>on shutdown")
>
>https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3a5cc90a4d1756072619fe511d07621bdef7f120
>
I worked with Filippo (+CC) on this patch.

IMHO it shouldn't do any harm. `sock_orphan` sets sk->sk_socket and 
sk_wq to NULL, and sets the SOCK_DEAD flag.

This patch sets the latter in the same place. All the other fields are 
not used by the transport->release() (at least in virtio-based 
transports), so from my perspective there is no real change.

What was your concern?

>BTW we also added a test to cover that scenario, so we should be fine 
>since the suite I run was fine.
Yep! Test runs fine.
>
>>Signed-off-by: Michal Luczaj <mhal@...x.co>
>>---
>>net/vmw_vsock/af_vsock.c | 3 ++-
>>1 file changed, 2 insertions(+), 1 deletion(-)
>>
>>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>>index 075695173648d3a4ecbd04e908130efdbb393b41..06250bb9afe2f253e96130b73554aae9151aaac1 100644
>>--- a/net/vmw_vsock/af_vsock.c
>>+++ b/net/vmw_vsock/af_vsock.c
>>@@ -824,13 +824,14 @@ static void __vsock_release(struct sock *sk, int level)
>>	 */
>>	lock_sock_nested(sk, level);
>>
>
>I would add a comment here to explain that we need to set it, so 
>vsock_remove_sock() called here some lines above, or by transports in 
>the release() callback (maybe in the future we can refactor it, and 
>call it only here) will remove the binding only if it's set, since the 
>release() is also called when de-assigning the transport.
>
>Thanks,
>Stefano
>
>>-	sock_orphan(sk);
>>+	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);
>>
>>-- 
>>2.48.1
>>
>

@Michal From the code POV I have no concerns, LGTM :)

Thanks,
Luigi