[PATCH] vsock: fix memory leak of rejected child sockets in vsock_accept()

[Chaithanya Lagisetty <nagachaithanya9911@gmail.com>]

When a listener socket carries an error (e.g. sk_err set by a connect()
issued on the socket before listen()), vsock_accept() dequeues the child
from the accept queue but rejects it. Previously it only marked the child
as rejected and relied on vsock_pending_work() to clean it up. However,
rejected child sockets created through virtio_transport and vsock_loopback
never reach that cleanup path, causing the child socket, along with its
LSM blob and transport-specific state, to leak permanently.

Fix this by releasing the child's references directly in vsock_accept()
on the reject path: remove it from the connected table and drop the
references taken by sk_alloc(), __vsock_insert_connected() and
vsock_enqueue_accept(), so the socket reaches vsock_sk_destruct() and is
freed. The now-unused 'rejected' flag and its handling in
vsock_pending_work() are removed.

Fixes: 06a8fc78367d ("VSOCK: Introduce virtio_vsock_common.ko")
Reported-by: syzbot+53515d23498d641e21ea@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=53515d23498d641e21ea
Signed-off-by: Chaithanya Lagisetty <nagachaithanya9911@gmail.com>
---
 include/net/af_vsock.h   |  4 +---
 net/vmw_vsock/af_vsock.c | 38 ++++++++++++++++++++------------------
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
index 30046a3c20f7..b70cea7f754f 100644
--- a/include/net/af_vsock.h
+++ b/include/net/af_vsock.h
@@ -53,12 +53,10 @@ struct vsock_sock {
 	 * for connection requests are placed in the pending list until they
 	 * are connected, at which point they are put in the accept queue list
 	 * so they can be accepted in accept().  If accept() cannot accept the
-	 * connection, it is marked as rejected so the cleanup function knows
-	 * to clean up the socket.
+	 * connection, the child is cleaned up directly in vsock_accept().
 	 */
 	struct list_head pending_links;
 	struct list_head accept_queue;
-	bool rejected;
 	struct delayed_work connect_work;
 	struct delayed_work pending_work;
 	struct delayed_work close_work;
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 622dbd046799..2084c88ac836 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -39,9 +39,9 @@
  * from the listener socket's pending list and enqueued in the listener
  * socket's accept queue.  Callers of accept(2) will accept connected sockets
  * from the listener socket's accept queue.  If the socket cannot be accepted
- * for some reason then it is marked rejected.  Once the connection is
- * accepted, it is owned by the user process and the responsibility for cleanup
- * falls with that user process.
+ * for some reason then it is cleaned up directly in vsock_accept().  Once the
+ * connection is accepted, it is owned by the user process and the
+ * responsibility for cleanup falls with that user process.
  *
  * - It is possible that these pending sockets will never reach the connected
  * state; in fact, we may never receive another packet after the connection
@@ -49,9 +49,7 @@
  * future, after some amount of time passes where a connection should have been
  * established.  This function ensures that the socket is off all lists so it
  * cannot be retrieved, then drops all references to the socket so it is cleaned
- * up (sock_put() -> sk_free() -> our sk_destruct implementation).  Note this
- * function will also cleanup rejected sockets, those that reach the connected
- * state but leave it before they have been accepted.
+ * up (sock_put() -> sk_free() -> our sk_destruct implementation).
  *
  * - Lock ordering for pending or accept queue sockets is:
  *
@@ -774,11 +772,11 @@ static void vsock_pending_work(struct work_struct *work)
 
 	if (vsock_is_pending(sk)) {
 		vsock_remove_pending(listener, sk);
-	} else if (!vsk->rejected) {
-		/* We are not on the pending list and accept() did not reject
-		 * us, so we must have been accepted by our user process.  We
-		 * just need to drop our references to the sockets and be on
-		 * our way.
+	} else {
+		/* We are not on the pending list, so we must have been accepted
+		 * by our user process (rejected sockets are cleaned up directly
+		 * in vsock_accept()).  We just need to drop our references to
+		 * the sockets and be on our way.
 		 */
 		cleanup = false;
 		goto out;
@@ -942,7 +940,6 @@ static struct sock *__vsock_create(struct net *net,
 	vsk->listener = NULL;
 	INIT_LIST_HEAD(&vsk->pending_links);
 	INIT_LIST_HEAD(&vsk->accept_queue);
-	vsk->rejected = false;
 	vsk->sent_request = false;
 	vsk->ignore_connecting_rst = false;
 	WRITE_ONCE(vsk->peer_shutdown, 0);
@@ -1919,14 +1916,19 @@ static int vsock_accept(struct socket *sock, struct socket *newsock,
 		vconnected = vsock_sk(connected);
 
 		/* If the listener socket has received an error, then we should
-		 * reject this socket and return.  Note that we simply mark the
-		 * socket rejected, drop our reference, and let the cleanup
-		 * function handle the cleanup; the fact that we found it in
-		 * the listener's accept queue guarantees that the cleanup
-		 * function hasn't run yet.
+		 * reject this socket and return.  The child was found on the
+		 * listener's accept queue, so it still holds the references
+		 * taken by sk_alloc(), __vsock_insert_connected() and
+		 * vsock_enqueue_accept().  Drop them here so the socket is
+		 * destroyed.  We cannot defer this to vsock_pending_work():
+		 * rejected child sockets created through virtio_transport and
+		 * vsock_loopback are not cleaned up by that worker, so the
+		 * child would otherwise leak permanently.
 		 */
 		if (err) {
-			vconnected->rejected = true;
+			vsock_remove_connected(vconnected);
+			connected->sk_state = TCP_CLOSE;
+			sock_put(connected);
 		} else {
 			newsock->state = SS_CONNECTED;
 			sock_graft(connected, newsock);
-- 
2.43.0