[PATCH net 0/5] vsock: Transport reassignment and error handling issues

STALE560d

13 messages, 2 authors, 2025-01-21 · open the first message on its own page

[PATCH net 0/5] vsock: Transport reassignment and error handling issues

From: Michal Luczaj <hidden>
Date: 2025-01-17 22:00:18

Series deals with two issues:
- socket reference count imbalance due to an unforgiving transport release
  (triggered by transport reassignment);
- unintentional API feature, a failing connect() making the socket
  impossible to use for any subsequent connect() attempts.

Signed-off-by: Michal Luczaj <redacted>
---
Michal Luczaj (5):
      vsock: Keep the binding until socket destruction
      vsock: Allow retrying on connect() failure
      vsock/test: Introduce vsock_bind()
      vsock/test: Add test for UAF due to socket unbinding
      vsock/test: Add test for connect() retries

 net/vmw_vsock/af_vsock.c         |  13 +++-
 tools/testing/vsock/util.c       |  56 +++++++---------
 tools/testing/vsock/util.h       |   1 +
 tools/testing/vsock/vsock_test.c | 136 ++++++++++++++++++++++++++++++++++-----
 4 files changed, 155 insertions(+), 51 deletions(-)
---
base-commit: 5d6a361dc01d823cb7c10697f16695d45a82b909
change-id: 20250116-vsock-transport-vs-autobind-2da49f1d5a0a

Best regards,
-- 
Michal Luczaj [off-list ref]

[PATCH net 5/5] vsock/test: Add test for connect() retries

From: Michal Luczaj <hidden>
Date: 2025-01-17 22:00:20

Deliberately fail a connect() attempt; expect error. Then verify that
subsequent attempt (using the same socket) can still succeed, rather than
fail outright.

Signed-off-by: Michal Luczaj <redacted>
---
 tools/testing/vsock/vsock_test.c | 52 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index 7f1916e23858b5ba407c34742a05b7bd6cfdcc10..712650f993e9df68ceb68ae02334c2775be09c7c 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c

@@ -1520,6 +1520,53 @@ static void test_seqpacket_transport_uaf_server(const struct test_opts *opts)
 	control_expectln("DONE");
 }
 
+static void test_stream_connect_retry_client(const struct test_opts *opts)
+{
+	struct sockaddr_vm addr = {
+		.svm_family = AF_VSOCK,
+		.svm_cid = opts->peer_cid,
+		.svm_port = opts->peer_port
+	};
+	int s, alen = sizeof(addr);
+
+	s = socket(AF_VSOCK, SOCK_STREAM, 0);
+	if (s < 0) {
+		perror("socket");
+		exit(EXIT_FAILURE);
+	}
+
+	if (!connect(s, (struct sockaddr *)&addr, alen)) {
+		fprintf(stderr, "Unexpected connect() #1 success\n");
+		exit(EXIT_FAILURE);
+	}
+
+	control_writeln("LISTEN");
+	control_expectln("LISTENING");
+
+	if (connect(s, (struct sockaddr *)&addr, alen)) {
+		perror("connect() #2");
+		exit(EXIT_FAILURE);
+	}
+
+	close(s);
+}
+
+static void test_stream_connect_retry_server(const struct test_opts *opts)
+{
+	int fd;
+
+	control_expectln("LISTEN");
+
+	fd = vsock_stream_accept(VMADDR_CID_ANY, opts->peer_port, NULL);
+	if (fd < 0) {
+		perror("accept");
+		exit(EXIT_FAILURE);
+	}
+
+	vsock_wait_remote_close(fd);
+	close(fd);
+}
+
 static struct test_case test_cases[] = {
 	{
 		.name = "SOCK_STREAM connection reset",

@@ -1655,6 +1702,11 @@ static struct test_case test_cases[] = {
 		.run_client = test_seqpacket_transport_uaf_client,
 		.run_server = test_seqpacket_transport_uaf_server,
 	},
+	{
+		.name = "connectible retry failed connect()",
+		.run_client = test_stream_connect_retry_client,
+		.run_server = test_stream_connect_retry_server,
+	},
 	{},
 };

-- 
2.47.1

[PATCH net 4/5] vsock/test: Add test for UAF due to socket unbinding

From: Michal Luczaj <hidden>
Date: 2025-01-17 22:00:21

Fail the autobind, then trigger a transport reassign. Socket might get
unbound from unbound_sockets, which then leads to a reference count
underflow.

Signed-off-by: Michal Luczaj <redacted>
---
 tools/testing/vsock/vsock_test.c | 67 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index 28a5083bbfd600cf84a1a85cec2f272ce6912dd3..7f1916e23858b5ba407c34742a05b7bd6cfdcc10 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c

@@ -1458,6 +1458,68 @@ static void test_stream_cred_upd_on_set_rcvlowat(const struct test_opts *opts)
 	test_stream_credit_update_test(opts, false);
 }
 
+#define MAX_PORT_RETRIES	24	/* net/vmw_vsock/af_vsock.c */
+#define VMADDR_CID_NONEXISTING	42
+
+/* Test attempts to trigger a transport release for an unbound socket. This can
+ * lead to a reference count mishandling.
+ */
+static void test_seqpacket_transport_uaf_client(const struct test_opts *opts)
+{
+	int sockets[MAX_PORT_RETRIES];
+	struct sockaddr_vm addr;
+	int s, i, alen;
+
+	s = vsock_bind(VMADDR_CID_LOCAL, VMADDR_PORT_ANY, SOCK_SEQPACKET);
+
+	alen = sizeof(addr);
+	if (getsockname(s, (struct sockaddr *)&addr, &alen)) {
+		perror("getsockname");
+		exit(EXIT_FAILURE);
+	}
+
+	for (i = 0; i < MAX_PORT_RETRIES; ++i)
+		sockets[i] = vsock_bind(VMADDR_CID_ANY, ++addr.svm_port,
+					SOCK_SEQPACKET);
+
+	close(s);
+	s = socket(AF_VSOCK, SOCK_SEQPACKET, 0);
+	if (s < 0) {
+		perror("socket");
+		exit(EXIT_FAILURE);
+	}
+
+	if (!connect(s, (struct sockaddr *)&addr, alen)) {
+		fprintf(stderr, "Unexpected connect() #1 success\n");
+		exit(EXIT_FAILURE);
+	}
+	/* connect() #1 failed: transport set, sk in unbound list. */
+
+	addr.svm_cid = VMADDR_CID_NONEXISTING;
+	if (!connect(s, (struct sockaddr *)&addr, alen)) {
+		fprintf(stderr, "Unexpected connect() #2 success\n");
+		exit(EXIT_FAILURE);
+	}
+	/* connect() #2 failed: transport unset, sk ref dropped? */
+
+	addr.svm_cid = VMADDR_CID_LOCAL;
+	addr.svm_port = VMADDR_PORT_ANY;
+
+	/* Vulnerable system may crash now. */
+	bind(s, (struct sockaddr *)&addr, alen);
+
+	close(s);
+	while (i--)
+		close(sockets[i]);
+
+	control_writeln("DONE");
+}
+
+static void test_seqpacket_transport_uaf_server(const struct test_opts *opts)
+{
+	control_expectln("DONE");
+}
+
 static struct test_case test_cases[] = {
 	{
 		.name = "SOCK_STREAM connection reset",

@@ -1588,6 +1650,11 @@ static struct test_case test_cases[] = {
 		.run_client = test_seqpacket_unsent_bytes_client,
 		.run_server = test_seqpacket_unsent_bytes_server,
 	},
+	{
+		.name = "connectible transport release use-after-free",
+		.run_client = test_seqpacket_transport_uaf_client,
+		.run_server = test_seqpacket_transport_uaf_server,
+	},
 	{},
 };

-- 
2.47.1

[PATCH net 3/5] vsock/test: Introduce vsock_bind()

From: Michal Luczaj <hidden>
Date: 2025-01-17 22:00:22

Add a helper for socket()+bind(). Adapt callers.

Signed-off-by: Michal Luczaj <redacted>
---
 tools/testing/vsock/util.c       | 56 +++++++++++++++++-----------------------
 tools/testing/vsock/util.h       |  1 +
 tools/testing/vsock/vsock_test.c | 17 +-----------
 3 files changed, 25 insertions(+), 49 deletions(-)

diff --git a/tools/testing/vsock/util.c b/tools/testing/vsock/util.c
index 34e9dac0a105f8aeb8c9af379b080d5ce8cb2782..31ee1767c8b73c05cfd219c3d520a677df6e66a6 100644
--- a/tools/testing/vsock/util.c
+++ b/tools/testing/vsock/util.c

@@ -96,33 +96,42 @@ void vsock_wait_remote_close(int fd)
 	close(epollfd);
 }
 
-/* Bind to <bind_port>, connect to <cid, port> and return the file descriptor. */
-int vsock_bind_connect(unsigned int cid, unsigned int port, unsigned int bind_port, int type)
+int vsock_bind(unsigned int cid, unsigned int port, int type)
 {
-	struct sockaddr_vm sa_client = {
-		.svm_family = AF_VSOCK,
-		.svm_cid = VMADDR_CID_ANY,
-		.svm_port = bind_port,
-	};
-	struct sockaddr_vm sa_server = {
+	struct sockaddr_vm sa = {
 		.svm_family = AF_VSOCK,
 		.svm_cid = cid,
 		.svm_port = port,
 	};
+	int fd;
 
-	int client_fd, ret;
-
-	client_fd = socket(AF_VSOCK, type, 0);
-	if (client_fd < 0) {
+	fd = socket(AF_VSOCK, type, 0);
+	if (fd < 0) {
 		perror("socket");
 		exit(EXIT_FAILURE);
 	}
 
-	if (bind(client_fd, (struct sockaddr *)&sa_client, sizeof(sa_client))) {
+	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa))) {
 		perror("bind");
 		exit(EXIT_FAILURE);
 	}
 
+	return fd;
+}
+
+/* Bind to <bind_port>, connect to <cid, port> and return the file descriptor. */
+int vsock_bind_connect(unsigned int cid, unsigned int port, unsigned int bind_port, int type)
+{
+	struct sockaddr_vm sa_server = {
+		.svm_family = AF_VSOCK,
+		.svm_cid = cid,
+		.svm_port = port,
+	};
+
+	int client_fd, ret;
+
+	client_fd = vsock_bind(VMADDR_CID_ANY, bind_port, type);
+
 	timeout_begin(TIMEOUT);
 	do {
 		ret = connect(client_fd, (struct sockaddr *)&sa_server, sizeof(sa_server));

@@ -192,28 +201,9 @@ int vsock_seqpacket_connect(unsigned int cid, unsigned int port)
 /* Listen on <cid, port> and return the file descriptor. */
 static int vsock_listen(unsigned int cid, unsigned int port, int type)
 {
-	union {
-		struct sockaddr sa;
-		struct sockaddr_vm svm;
-	} addr = {
-		.svm = {
-			.svm_family = AF_VSOCK,
-			.svm_port = port,
-			.svm_cid = cid,
-		},
-	};
 	int fd;
 
-	fd = socket(AF_VSOCK, type, 0);
-	if (fd < 0) {
-		perror("socket");
-		exit(EXIT_FAILURE);
-	}
-
-	if (bind(fd, &addr.sa, sizeof(addr.svm)) < 0) {
-		perror("bind");
-		exit(EXIT_FAILURE);
-	}
+	fd = vsock_bind(cid, port, type);
 
 	if (listen(fd, 1) < 0) {
 		perror("listen");

diff --git a/tools/testing/vsock/util.h b/tools/testing/vsock/util.h
index ba84d296d8b71e1bcba2abdad337e07aac45e75e..7736594a15d29449d98bd1e9e19c3acd1a623443 100644
--- a/tools/testing/vsock/util.h
+++ b/tools/testing/vsock/util.h

@@ -43,6 +43,7 @@ int vsock_connect(unsigned int cid, unsigned int port, int type);
 int vsock_accept(unsigned int cid, unsigned int port,
 		 struct sockaddr_vm *clientaddrp, int type);
 int vsock_stream_connect(unsigned int cid, unsigned int port);
+int vsock_bind(unsigned int cid, unsigned int port, int type);
 int vsock_bind_connect(unsigned int cid, unsigned int port,
 		       unsigned int bind_port, int type);
 int vsock_seqpacket_connect(unsigned int cid, unsigned int port);

diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index 48f17641ca504316d1199926149c9bd62eb2921d..28a5083bbfd600cf84a1a85cec2f272ce6912dd3 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c

@@ -108,24 +108,9 @@ static void test_stream_bind_only_client(const struct test_opts *opts)
 
 static void test_stream_bind_only_server(const struct test_opts *opts)
 {
-	union {
-		struct sockaddr sa;
-		struct sockaddr_vm svm;
-	} addr = {
-		.svm = {
-			.svm_family = AF_VSOCK,
-			.svm_port = opts->peer_port,
-			.svm_cid = VMADDR_CID_ANY,
-		},
-	};
 	int fd;
 
-	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
-
-	if (bind(fd, &addr.sa, sizeof(addr.svm)) < 0) {
-		perror("bind");
-		exit(EXIT_FAILURE);
-	}
+	fd = vsock_bind(VMADDR_CID_ANY, opts->peer_port, SOCK_STREAM);
 
 	/* Notify the client that the server is ready */
 	control_writeln("BIND");

-- 
2.47.1

[PATCH net 1/5] vsock: Keep the binding until socket destruction

From: Michal Luczaj <hidden>
Date: 2025-01-17 22:00:24

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")
Signed-off-by: Michal Luczaj <redacted>
---
 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 fa9d1b49599bf219bdf9486741582cc8c547a354..cfe18bc8fdbe7ced073c6b3644d635fdbfa02610 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);

-- 
2.47.1

[PATCH net 2/5] vsock: Allow retrying on connect() failure

From: Michal Luczaj <hidden>
Date: 2025-01-17 22:00:25

sk_err is set when a (connectible) connect() fails. Effectively, this makes
an otherwise still healthy SS_UNCONNECTED socket impossible to use for any
subsequent connection attempts.

Clear sk_err upon trying to establish a connection.

Fixes: d021c344051a ("VSOCK: Introduce VM Sockets")
Signed-off-by: Michal Luczaj <redacted>
---
 net/vmw_vsock/af_vsock.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index cfe18bc8fdbe7ced073c6b3644d635fdbfa02610..075695173648d3a4ecbd04e908130efdbb393b41 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c

@@ -1523,6 +1523,11 @@ static int vsock_connect(struct socket *sock, struct sockaddr *addr,
 		if (err < 0)
 			goto out;
 
+		/* sk_err might have been set as a result of an earlier
+		 * (failed) connect attempt.
+		 */
+		sk->sk_err = 0;
+
 		/* Mark sock as connecting and set the error code to in
 		 * progress in case this is a non-blocking connect.
 		 */

-- 
2.47.1

Re: [PATCH net 1/5] vsock: Keep the binding until socket destruction

From: Stefano Garzarella <sgarzare@redhat.com>
Date: 2025-01-20 10:05:39

On Fri, Jan 17, 2025 at 10:59:41PM +0100, Michal Luczaj wrote:

quoted hunk

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")
Signed-off-by: Michal Luczaj <redacted>
---
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 fa9d1b49599bf219bdf9486741582cc8c547a354..cfe18bc8fdbe7ced073c6b3644d635fdbfa02610 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);
+

yeah, great fix!

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

quoted hunk

	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);

-- 
2.47.1

Re: [PATCH net 2/5] vsock: Allow retrying on connect() failure

From: Stefano Garzarella <sgarzare@redhat.com>
Date: 2025-01-20 10:06:09

On Fri, Jan 17, 2025 at 10:59:42PM +0100, Michal Luczaj wrote:

quoted hunk

sk_err is set when a (connectible) connect() fails. Effectively, this makes
an otherwise still healthy SS_UNCONNECTED socket impossible to use for any
subsequent connection attempts.

Clear sk_err upon trying to establish a connection.

Fixes: d021c344051a ("VSOCK: Introduce VM Sockets")
Signed-off-by: Michal Luczaj <redacted>
---
net/vmw_vsock/af_vsock.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index cfe18bc8fdbe7ced073c6b3644d635fdbfa02610..075695173648d3a4ecbd04e908130efdbb393b41 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c

@@ -1523,6 +1523,11 @@ static int vsock_connect(struct socket *sock, struct sockaddr *addr,

		if (err < 0)
			goto out;

+		/* sk_err might have been set as a result of an earlier
+		 * (failed) connect attempt.
+		 */
+		sk->sk_err = 0;
+

LGTM!

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

		/* Mark sock as connecting and set the error code to in
		 * progress in case this is a non-blocking connect.
		 */

-- 
2.47.1

Re: [PATCH net 3/5] vsock/test: Introduce vsock_bind()

From: Stefano Garzarella <sgarzare@redhat.com>
Date: 2025-01-20 10:06:37

On Fri, Jan 17, 2025 at 10:59:43PM +0100, Michal Luczaj wrote:

Add a helper for socket()+bind(). Adapt callers.

Signed-off-by: Michal Luczaj <redacted>
---
tools/testing/vsock/util.c       | 56 +++++++++++++++++-----------------------
tools/testing/vsock/util.h       |  1 +
tools/testing/vsock/vsock_test.c | 17 +-----------
3 files changed, 25 insertions(+), 49 deletions(-)

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

quoted hunk

diff --git a/tools/testing/vsock/util.c b/tools/testing/vsock/util.c
index 34e9dac0a105f8aeb8c9af379b080d5ce8cb2782..31ee1767c8b73c05cfd219c3d520a677df6e66a6 100644
--- a/tools/testing/vsock/util.c
+++ b/tools/testing/vsock/util.c

@@ -96,33 +96,42 @@ void vsock_wait_remote_close(int fd)

	close(epollfd);
}

-/* Bind to <bind_port>, connect to <cid, port> and return the file descriptor. */
-int vsock_bind_connect(unsigned int cid, unsigned int port, unsigned int bind_port, int type)
+int vsock_bind(unsigned int cid, unsigned int port, int type)
{
-	struct sockaddr_vm sa_client = {
-		.svm_family = AF_VSOCK,
-		.svm_cid = VMADDR_CID_ANY,
-		.svm_port = bind_port,
-	};
-	struct sockaddr_vm sa_server = {
+	struct sockaddr_vm sa = {
		.svm_family = AF_VSOCK,
		.svm_cid = cid,
		.svm_port = port,
	};
+	int fd;

-	int client_fd, ret;
-
-	client_fd = socket(AF_VSOCK, type, 0);
-	if (client_fd < 0) {
+	fd = socket(AF_VSOCK, type, 0);
+	if (fd < 0) {
		perror("socket");
		exit(EXIT_FAILURE);
	}

-	if (bind(client_fd, (struct sockaddr *)&sa_client, sizeof(sa_client))) {
+	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa))) {
		perror("bind");
		exit(EXIT_FAILURE);
	}

+	return fd;
+}
+
+/* Bind to <bind_port>, connect to <cid, port> and return the file descriptor. */
+int vsock_bind_connect(unsigned int cid, unsigned int port, unsigned int bind_port, int type)
+{
+	struct sockaddr_vm sa_server = {
+		.svm_family = AF_VSOCK,
+		.svm_cid = cid,
+		.svm_port = port,
+	};
+
+	int client_fd, ret;
+
+	client_fd = vsock_bind(VMADDR_CID_ANY, bind_port, type);
+
	timeout_begin(TIMEOUT);
	do {
		ret = connect(client_fd, (struct sockaddr *)&sa_server, sizeof(sa_server));

@@ -192,28 +201,9 @@ int vsock_seqpacket_connect(unsigned int cid, unsigned int port)

/* Listen on <cid, port> and return the file descriptor. */
static int vsock_listen(unsigned int cid, unsigned int port, int type)
{
-	union {
-		struct sockaddr sa;
-		struct sockaddr_vm svm;
-	} addr = {
-		.svm = {
-			.svm_family = AF_VSOCK,
-			.svm_port = port,
-			.svm_cid = cid,
-		},
-	};
	int fd;

-	fd = socket(AF_VSOCK, type, 0);
-	if (fd < 0) {
-		perror("socket");
-		exit(EXIT_FAILURE);
-	}
-
-	if (bind(fd, &addr.sa, sizeof(addr.svm)) < 0) {
-		perror("bind");
-		exit(EXIT_FAILURE);
-	}
+	fd = vsock_bind(cid, port, type);

	if (listen(fd, 1) < 0) {
		perror("listen");

diff --git a/tools/testing/vsock/util.h b/tools/testing/vsock/util.h
index ba84d296d8b71e1bcba2abdad337e07aac45e75e..7736594a15d29449d98bd1e9e19c3acd1a623443 100644
--- a/tools/testing/vsock/util.h
+++ b/tools/testing/vsock/util.h

@@ -43,6 +43,7 @@ int vsock_connect(unsigned int cid, unsigned int port, int type);

int vsock_accept(unsigned int cid, unsigned int port,
		 struct sockaddr_vm *clientaddrp, int type);
int vsock_stream_connect(unsigned int cid, unsigned int port);
+int vsock_bind(unsigned int cid, unsigned int port, int type);
int vsock_bind_connect(unsigned int cid, unsigned int port,
		       unsigned int bind_port, int type);
int vsock_seqpacket_connect(unsigned int cid, unsigned int port);

diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index 48f17641ca504316d1199926149c9bd62eb2921d..28a5083bbfd600cf84a1a85cec2f272ce6912dd3 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c

@@ -108,24 +108,9 @@ static void test_stream_bind_only_client(const struct test_opts *opts)

static void test_stream_bind_only_server(const struct test_opts *opts)
{
-	union {
-		struct sockaddr sa;
-		struct sockaddr_vm svm;
-	} addr = {
-		.svm = {
-			.svm_family = AF_VSOCK,
-			.svm_port = opts->peer_port,
-			.svm_cid = VMADDR_CID_ANY,
-		},
-	};
	int fd;

-	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
-
-	if (bind(fd, &addr.sa, sizeof(addr.svm)) < 0) {
-		perror("bind");
-		exit(EXIT_FAILURE);
-	}
+	fd = vsock_bind(VMADDR_CID_ANY, opts->peer_port, SOCK_STREAM);

	/* Notify the client that the server is ready */
	control_writeln("BIND");

-- 
2.47.1

Re: [PATCH net 4/5] vsock/test: Add test for UAF due to socket unbinding

From: Stefano Garzarella <sgarzare@redhat.com>
Date: 2025-01-20 10:20:14

On Fri, Jan 17, 2025 at 10:59:44PM +0100, Michal Luczaj wrote:

quoted hunk

Fail the autobind, then trigger a transport reassign. Socket might get
unbound from unbound_sockets, which then leads to a reference count
underflow.

Signed-off-by: Michal Luczaj <redacted>
---
tools/testing/vsock/vsock_test.c | 67 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 67 insertions(+)

diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index 28a5083bbfd600cf84a1a85cec2f272ce6912dd3..7f1916e23858b5ba407c34742a05b7bd6cfdcc10 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c

@@ -1458,6 +1458,68 @@ static void test_stream_cred_upd_on_set_rcvlowat(const struct test_opts *opts)

	test_stream_credit_update_test(opts, false);
}

+#define MAX_PORT_RETRIES	24	/* net/vmw_vsock/af_vsock.c */
+#define VMADDR_CID_NONEXISTING	42

I would suggest a slightly bigger and weirder CID, this might actually
exist, (e.g. 4242424242)

+
+/* Test attempts to trigger a transport release for an unbound socket. This can
+ * lead to a reference count mishandling.
+ */
+static void test_seqpacket_transport_uaf_client(const struct test_opts *opts)
+{
+	int sockets[MAX_PORT_RETRIES];
+	struct sockaddr_vm addr;
+	int s, i, alen;
+
+	s = vsock_bind(VMADDR_CID_LOCAL, VMADDR_PORT_ANY, SOCK_SEQPACKET);

In my suite this test failed because I have instances where I run it
without vsock_loopback loaded.

26 - connectible transport release use-after-free...bind: Cannot assign requested address

Is it important to use VMADDR_CID_LOCAL or can we use VMADDR_CID_ANY?

+
+	alen = sizeof(addr);
+	if (getsockname(s, (struct sockaddr *)&addr, &alen)) {
+		perror("getsockname");
+		exit(EXIT_FAILURE);
+	}
+
+	for (i = 0; i < MAX_PORT_RETRIES; ++i)
+		sockets[i] = vsock_bind(VMADDR_CID_ANY, ++addr.svm_port,
+					SOCK_SEQPACKET);
+
+	close(s);
+	s = socket(AF_VSOCK, SOCK_SEQPACKET, 0);
+	if (s < 0) {
+		perror("socket");
+		exit(EXIT_FAILURE);
+	}
+
+	if (!connect(s, (struct sockaddr *)&addr, alen)) {
+		fprintf(stderr, "Unexpected connect() #1 success\n");
+		exit(EXIT_FAILURE);
+	}
+	/* connect() #1 failed: transport set, sk in unbound list. */
+
+	addr.svm_cid = VMADDR_CID_NONEXISTING;
+	if (!connect(s, (struct sockaddr *)&addr, alen)) {
+		fprintf(stderr, "Unexpected connect() #2 success\n");
+		exit(EXIT_FAILURE);
+	}
+	/* connect() #2 failed: transport unset, sk ref dropped? */
+
+	addr.svm_cid = VMADDR_CID_LOCAL;

Ditto.

+	addr.svm_port = VMADDR_PORT_ANY;
+
+	/* Vulnerable system may crash now. */
+	bind(s, (struct sockaddr *)&addr, alen);

Should we check the return of this function or do we not care whether
it fails or not?

quoted hunk

+
+	close(s);
+	while (i--)
+		close(sockets[i]);
+
+	control_writeln("DONE");
+}
+
+static void test_seqpacket_transport_uaf_server(const struct test_opts *opts)
+{
+	control_expectln("DONE");
+}
+
static struct test_case test_cases[] = {
	{
		.name = "SOCK_STREAM connection reset",

@@ -1588,6 +1650,11 @@ static struct test_case test_cases[] = {

		.run_client = test_seqpacket_unsent_bytes_client,
		.run_server = test_seqpacket_unsent_bytes_server,
	},
+	{
+		.name = "connectible transport release use-after-free",

If it doesn't matter that it is SOCK_STREAM or SOCK_SEQPACKET, I would
rather test SOCK_STREAM because it is more common.

Anyway, here I would specify which of the two we are testing for
accordance.

"SOCK_STREAM transport release use-after-free".

Tanks for adding this test!
Stefano

+		.run_client = test_seqpacket_transport_uaf_client,
+		.run_server = test_seqpacket_transport_uaf_server,
+	},
	{},
};


-- 
2.47.1

Re: [PATCH net 5/5] vsock/test: Add test for connect() retries

From: Stefano Garzarella <sgarzare@redhat.com>
Date: 2025-01-20 10:24:38

On Fri, Jan 17, 2025 at 10:59:45PM +0100, Michal Luczaj wrote:

quoted hunk

Deliberately fail a connect() attempt; expect error. Then verify that
subsequent attempt (using the same socket) can still succeed, rather than
fail outright.

Signed-off-by: Michal Luczaj <redacted>
---
tools/testing/vsock/vsock_test.c | 52 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)

diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index 7f1916e23858b5ba407c34742a05b7bd6cfdcc10..712650f993e9df68ceb68ae02334c2775be09c7c 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c

@@ -1520,6 +1520,53 @@ static void test_seqpacket_transport_uaf_server(const struct test_opts *opts)

	control_expectln("DONE");
}

+static void test_stream_connect_retry_client(const struct test_opts *opts)
+{
+	struct sockaddr_vm addr = {
+		.svm_family = AF_VSOCK,
+		.svm_cid = opts->peer_cid,
+		.svm_port = opts->peer_port
+	};
+	int s, alen = sizeof(addr);
+
+	s = socket(AF_VSOCK, SOCK_STREAM, 0);
+	if (s < 0) {
+		perror("socket");
+		exit(EXIT_FAILURE);
+	}
+
+	if (!connect(s, (struct sockaddr *)&addr, alen)) {
+		fprintf(stderr, "Unexpected connect() #1 success\n");
+		exit(EXIT_FAILURE);
+	}
+
+	control_writeln("LISTEN");
+	control_expectln("LISTENING");
+
+	if (connect(s, (struct sockaddr *)&addr, alen)) {
+		perror("connect() #2");
+		exit(EXIT_FAILURE);
+	}

What about using the timeout_begin()/timeout_end() we used in all other
places?

quoted hunk

+
+	close(s);
+}
+
+static void test_stream_connect_retry_server(const struct test_opts *opts)
+{
+	int fd;
+
+	control_expectln("LISTEN");
+
+	fd = vsock_stream_accept(VMADDR_CID_ANY, opts->peer_port, NULL);
+	if (fd < 0) {
+		perror("accept");
+		exit(EXIT_FAILURE);
+	}
+
+	vsock_wait_remote_close(fd);
+	close(fd);
+}
+
static struct test_case test_cases[] = {
	{
		.name = "SOCK_STREAM connection reset",

@@ -1655,6 +1702,11 @@ static struct test_case test_cases[] = {

		.run_client = test_seqpacket_transport_uaf_client,
		.run_server = test_seqpacket_transport_uaf_server,
	},
+	{
+		.name = "connectible retry failed connect()",

"SOCK_STREAM retry failed connect()"

(and if you want add a comment on top of the test the in the current
implementation the same path is shared with other connectible socket
like SOCK_SEQPACKET)

The rest LGTM.

Thanks,
Stefano

+		.run_client = test_stream_connect_retry_client,
+		.run_server = test_stream_connect_retry_server,
+	},
	{},
};


-- 
2.47.1

Re: [PATCH net 5/5] vsock/test: Add test for connect() retries

From: Michal Luczaj <hidden>
Date: 2025-01-21 13:20:35

On 1/20/25 11:24, Stefano Garzarella wrote:

On Fri, Jan 17, 2025 at 10:59:45PM +0100, Michal Luczaj wrote:

quoted

+static void test_stream_connect_retry_client(const struct test_opts *opts)
+{
+	struct sockaddr_vm addr = {
+		.svm_family = AF_VSOCK,
+		.svm_cid = opts->peer_cid,
+		.svm_port = opts->peer_port
+	};
+	int s, alen = sizeof(addr);
+
+	s = socket(AF_VSOCK, SOCK_STREAM, 0);
+	if (s < 0) {
+		perror("socket");
+		exit(EXIT_FAILURE);
+	}
+
+	if (!connect(s, (struct sockaddr *)&addr, alen)) {
+		fprintf(stderr, "Unexpected connect() #1 success\n");
+		exit(EXIT_FAILURE);
+	}
+
+	control_writeln("LISTEN");
+	control_expectln("LISTENING");
+
+	if (connect(s, (struct sockaddr *)&addr, alen)) {
+		perror("connect() #2");
+		exit(EXIT_FAILURE);
+	}

What about using the timeout_begin()/timeout_end() we used in all other
places?

Oh, right!

Thanks for the review, all remaining comments addressed.

Re: [PATCH net 4/5] vsock/test: Add test for UAF due to socket unbinding

From: Michal Luczaj <hidden>
Date: 2025-01-21 14:48:08

On 1/20/25 11:20, Stefano Garzarella wrote:

On Fri, Jan 17, 2025 at 10:59:44PM +0100, Michal Luczaj wrote:

quoted

+#define MAX_PORT_RETRIES	24	/* net/vmw_vsock/af_vsock.c */
+#define VMADDR_CID_NONEXISTING	42

I would suggest a slightly bigger and weirder CID, this might actually
exist, (e.g. 4242424242)

Ok. I'll try to avoid the whole VMADDR_CID_NONEXISTING. See below.

quoted

+/* Test attempts to trigger a transport release for an unbound socket. This can
+ * lead to a reference count mishandling.
+ */
+static void test_seqpacket_transport_uaf_client(const struct test_opts *opts)
+{
+	int sockets[MAX_PORT_RETRIES];
+	struct sockaddr_vm addr;
+	int s, i, alen;
+
+	s = vsock_bind(VMADDR_CID_LOCAL, VMADDR_PORT_ANY, SOCK_SEQPACKET);

In my suite this test failed because I have instances where I run it
without vsock_loopback loaded.

26 - connectible transport release use-after-free...bind: Cannot assign requested address

Is it important to use VMADDR_CID_LOCAL or can we use VMADDR_CID_ANY?

To force the transport release on a live socket (via transport
reassignment) we need to switch between two transports (but it's okay if
the other transport turns out to be NULL). So, considering your setup(s),
is it reasonable to expect VMADDR_CID_ANY and VMADDR_CID_HOST bringing in
different transports (as in: different pointers)? Any other combination?

I've tried that in v2:
https://lore.kernel.org/netdev/20250121-vsock-transport-vs-autobind-v2-0-aad6069a4e8c@rbox.co

quoted

+	/* Vulnerable system may crash now. */
+	bind(s, (struct sockaddr *)&addr, alen);

Should we check the return of this function or do we not care whether
it fails or not?

We don't really care, but I guess it just looks bad. I'll fix that.

Thanks,
Michal

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help