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: <142e66e5-7080-41d9-ac5c-c392dfa68d2a@rbox.co>
Date: Wed, 5 Feb 2025 01:11:35 +0100
From: Michal Luczaj <mhal@...x.co>
To: Stefano Garzarella <sgarzare@...hat.com>,
 Luigi Leonardi <leonardi@...hat.com>
Cc: "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 2/4/25 17:00, Stefano Garzarella wrote:
> On Tue, Feb 04, 2025 at 04:44:13PM +0100, Luigi Leonardi wrote:
>> 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?
> 
> My concern was more about calling `vsock_remove_sock()` in 
> virtio_transport_recv_connected:
> 
> I mean this block:
> 	case VIRTIO_VSOCK_OP_SHUTDOWN:
> 		if (le32_to_cpu(hdr->flags) & VIRTIO_VSOCK_SHUTDOWN_RCV)
> 			vsk->peer_shutdown |= RCV_SHUTDOWN;
> 		if (le32_to_cpu(hdr->flags) & VIRTIO_VSOCK_SHUTDOWN_SEND)
> 			vsk->peer_shutdown |= SEND_SHUTDOWN;
> 		if (vsk->peer_shutdown == SHUTDOWN_MASK) {
> 			if (vsock_stream_has_data(vsk) <= 0 && !sock_flag(sk, SOCK_DONE)) {
> 				(void)virtio_transport_reset(vsk, NULL);
> 				virtio_transport_do_close(vsk, true);
> 			}
> 			/* Remove this socket anyway because the remote peer sent
> 			 * the shutdown. This way a new connection will succeed
> 			 * if the remote peer uses the same source port,
> 			 * even if the old socket is still unreleased, but now disconnected.
> 			 */
> 			vsock_remove_sock(vsk);
> 		}
> 
> After commit fcdd2242c023 ("vsock: Keep the binding until socket 
> destruction") calling `vsock_remove_sock` without SOCK_DEAD set, removes 
> the socket only from the connected list.
> 
> So, IMHO there is a real change, but I'm not sure if it's an issue or 
> not, since the issue fixed by commit 3a5cc90a4d17 ("vsock/virtio: remove 
> socket from connected/bound list on shutdown") was more about the remote 
> port IIRC, so that should only be affected by the connected list, which 
> is stll touched now.

I agree, not an issue. But maybe it's worth replacing
vsock_remove_sock(vsk) with vsock_remove_connected(vsk) to better convey
what kind of removal we're talking about here?

Michal