netdev - Re: [PATCH net 1/4] virtio/vsock: Fix accept

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: <dcboxomaif7b3qt3ofls4wvnobclv3onb32ynnq5uchzqoz6kg@5htm6qcbvmpg>
Date: Thu, 7 Nov 2024 11:15:20 +0100
From: Stefano Garzarella <sgarzare@...hat.com>
To: Michal Luczaj <mhal@...x.co>
Cc: Stefan Hajnoczi <stefanha@...hat.com>, 
	"Michael S. Tsirkin" <mst@...hat.com>, Jason Wang <jasowang@...hat.com>, 
	Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, Eugenio Pérez <eperezma@...hat.com>, 
	"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>, 
	Jia He <justin.he@....com>, Arseniy Krasnov <avkrasnov@...utedevices.com>, 
	Dmitry Torokhov <dtor@...are.com>, Andy King <acking@...are.com>, 
	George Zhang <georgezhang@...are.com>, kvm@...r.kernel.org, virtualization@...ts.linux.dev, 
	netdev@...r.kernel.org
Subject: Re: [PATCH net 1/4] virtio/vsock: Fix accept_queue memory leak

On Wed, Nov 06, 2024 at 06:51:18PM +0100, Michal Luczaj wrote:
>As the final stages of socket destruction may be delayed, it is possible
>that virtio_transport_recv_listen() will be called after the accept_queue
>has been flushed, but before the SOCK_DONE flag has been set. As a result,
>sockets enqueued after the flush would remain unremoved, leading to a
>memory leak.
>
>vsock_release
>  __vsock_release
>    lock
>    virtio_transport_release
>      virtio_transport_close
>        schedule_delayed_work(close_work)
>    sk_shutdown = SHUTDOWN_MASK
>(!) flush accept_queue
>    release
>                                        virtio_transport_recv_pkt
>                                          vsock_find_bound_socket
>                                          lock
>                                          if flag(SOCK_DONE) return
>                                          virtio_transport_recv_listen
>                                            child = vsock_create_connected
>                                      (!)   vsock_enqueue_accept(child)
>                                          release
>close_work
>  lock
>  virtio_transport_do_close
>    set_flag(SOCK_DONE)
>    virtio_transport_remove_sock
>      vsock_remove_sock
>        vsock_remove_bound
>  release
>
>Introduce a sk_shutdown check to disallow vsock_enqueue_accept() during
>socket destruction.
>
>unreferenced object 0xffff888109e3f800 (size 2040):
>  comm "kworker/5:2", pid 371, jiffies 4294940105
>  hex dump (first 32 bytes):
>    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>    28 00 0b 40 00 00 00 00 00 00 00 00 00 00 00 00  (..@............
>  backtrace (crc 9e5f4e84):
>    [<ffffffff81418ff1>] kmem_cache_alloc_noprof+0x2c1/0x360
>    [<ffffffff81d27aa0>] sk_prot_alloc+0x30/0x120
>    [<ffffffff81d2b54c>] sk_alloc+0x2c/0x4b0
>    [<ffffffff81fe049a>] __vsock_create.constprop.0+0x2a/0x310
>    [<ffffffff81fe6d6c>] virtio_transport_recv_pkt+0x4dc/0x9a0
>    [<ffffffff81fe745d>] vsock_loopback_work+0xfd/0x140
>    [<ffffffff810fc6ac>] process_one_work+0x20c/0x570
>    [<ffffffff810fce3f>] worker_thread+0x1bf/0x3a0
>    [<ffffffff811070dd>] kthread+0xdd/0x110
>    [<ffffffff81044fdd>] ret_from_fork+0x2d/0x50
>    [<ffffffff8100785a>] ret_from_fork_asm+0x1a/0x30
>
>Fixes: 3fe356d58efa ("vsock/virtio: discard packets only when socket is really closed")
>Signed-off-by: Michal Luczaj <mhal@...x.co>
>---
> net/vmw_vsock/virtio_transport_common.c | 8 ++++++++
> 1 file changed, 8 insertions(+)

Reviewed-by: Stefano Garzarella <sgarzare@...hat.com>

>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index ccbd2bc0d2109aea4f19e79a0438f85893e1d89c..cd075f608d4f6f48f894543e5e9c966d3e5f22df 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -1512,6 +1512,14 @@ virtio_transport_recv_listen(struct sock *sk, struct sk_buff *skb,
> 		return -ENOMEM;
> 	}
>
>+	/* __vsock_release() might have already flushed accept_queue.
>+	 * Subsequent enqueues would lead to a memory leak.
>+	 */
>+	if (sk->sk_shutdown == SHUTDOWN_MASK) {
>+		virtio_transport_reset_no_sock(t, skb);
>+		return -ESHUTDOWN;
>+	}
>+
> 	child = vsock_create_connected(sk);
> 	if (!child) {
> 		virtio_transport_reset_no_sock(t, skb);
>
>-- 
>2.46.2
>