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

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

[Michal Luczaj]

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 <mhal@rbox.co>
---
Changes in v2:
- Introduce vsock_connect_fd(), simplify the tests, stick to SOCK_STREAM,
  collect Reviewed-by (Stefano)
- Link to v1: https://lore.kernel.org/r/20250117-vsock-transport-vs-autobind-v1-0-c802c803762d@rbox.co

---
Michal Luczaj (6):
      vsock: Keep the binding until socket destruction
      vsock: Allow retrying on connect() failure
      vsock/test: Introduce vsock_bind()
      vsock/test: Introduce vsock_connect_fd()
      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       |  87 +++++++++++-----------------
 tools/testing/vsock/util.h       |   2 +
 tools/testing/vsock/vsock_test.c | 122 ++++++++++++++++++++++++++++++++++-----
 4 files changed, 152 insertions(+), 72 deletions(-)
---
base-commit: d640627663bfe7d8963c7615316d7d4ef60f3b0b
change-id: 20250116-vsock-transport-vs-autobind-2da49f1d5a0a

Best regards,
-- 
Michal Luczaj <mhal@rbox.co>

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

[Michal Luczaj]

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")
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 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.48.1

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

[Michal Luczaj]

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")
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 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.48.1

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

[Michal Luczaj]

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

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 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.48.1

[PATCH net v2 4/6] vsock/test: Introduce vsock_connect_fd()

[Michal Luczaj]

Distill timeout-guarded vsock_connect_fd(). Adapt callers.

Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 tools/testing/vsock/util.c | 45 +++++++++++++++++----------------------------
 tools/testing/vsock/util.h |  1 +
 2 files changed, 18 insertions(+), 28 deletions(-)

diff --git a/tools/testing/vsock/util.c b/tools/testing/vsock/util.c
index 31ee1767c8b73c05cfd219c3d520a677df6e66a6..7f7e45a6596c19b09176ea2851a490cdac0f115b 100644
--- a/tools/testing/vsock/util.c
+++ b/tools/testing/vsock/util.c
@@ -119,27 +119,33 @@ int vsock_bind(unsigned int cid, unsigned int port, int type)
 	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)
+int vsock_connect_fd(int fd, unsigned int cid, unsigned int port)
 {
-	struct sockaddr_vm sa_server = {
+	struct sockaddr_vm sa = {
 		.svm_family = AF_VSOCK,
 		.svm_cid = cid,
 		.svm_port = port,
 	};
-
-	int client_fd, ret;
-
-	client_fd = vsock_bind(VMADDR_CID_ANY, bind_port, type);
+	int ret;
 
 	timeout_begin(TIMEOUT);
 	do {
-		ret = connect(client_fd, (struct sockaddr *)&sa_server, sizeof(sa_server));
+		ret = connect(fd, (struct sockaddr *)&sa, sizeof(sa));
 		timeout_check("connect");
 	} while (ret < 0 && errno == EINTR);
 	timeout_end();
 
-	if (ret < 0) {
+	return ret;
+}
+
+/* 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 client_fd;
+
+	client_fd = vsock_bind(VMADDR_CID_ANY, bind_port, type);
+
+	if (vsock_connect_fd(client_fd, cid, port)) {
 		perror("connect");
 		exit(EXIT_FAILURE);
 	}
@@ -150,17 +156,6 @@ int vsock_bind_connect(unsigned int cid, unsigned int port, unsigned int bind_po
 /* Connect to <cid, port> and return the file descriptor. */
 int vsock_connect(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 ret;
 	int fd;
 
 	control_expectln("LISTENING");
@@ -171,20 +166,14 @@ int vsock_connect(unsigned int cid, unsigned int port, int type)
 		exit(EXIT_FAILURE);
 	}
 
-	timeout_begin(TIMEOUT);
-	do {
-		ret = connect(fd, &addr.sa, sizeof(addr.svm));
-		timeout_check("connect");
-	} while (ret < 0 && errno == EINTR);
-	timeout_end();
-
-	if (ret < 0) {
+	if (vsock_connect_fd(fd, cid, port)) {
 		int old_errno = errno;
 
 		close(fd);
 		fd = -1;
 		errno = old_errno;
 	}
+
 	return fd;
 }
 
diff --git a/tools/testing/vsock/util.h b/tools/testing/vsock/util.h
index 7736594a15d29449d98bd1e9e19c3acd1a623443..817e11e483cd6596dd32d16061d801a66091c973 100644
--- a/tools/testing/vsock/util.h
+++ b/tools/testing/vsock/util.h
@@ -39,6 +39,7 @@ struct test_case {
 void init_signals(void);
 unsigned int parse_cid(const char *str);
 unsigned int parse_port(const char *str);
+int vsock_connect_fd(int fd, unsigned int cid, unsigned int port);
 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);

-- 
2.48.1

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

[Michal Luczaj]

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 <mhal@rbox.co>
---
 tools/testing/vsock/vsock_test.c | 58 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index 28a5083bbfd600cf84a1a85cec2f272ce6912dd3..572e0fd3e5a841f846fb304a24192f63d57ec052 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -1458,6 +1458,59 @@ 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 */
+
+/* Test attempts to trigger a transport release for an unbound socket. This can
+ * lead to a reference count mishandling.
+ */
+static void test_stream_transport_uaf_client(const struct test_opts *opts)
+{
+	int sockets[MAX_PORT_RETRIES];
+	struct sockaddr_vm addr;
+	int fd, i, alen;
+
+	fd = vsock_bind(VMADDR_CID_ANY, VMADDR_PORT_ANY, SOCK_STREAM);
+
+	alen = sizeof(addr);
+	if (getsockname(fd, (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_STREAM);
+
+	close(fd);
+	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
+	if (fd < 0) {
+		perror("socket");
+		exit(EXIT_FAILURE);
+	}
+
+	if (!vsock_connect_fd(fd, addr.svm_cid, addr.svm_port)) {
+		perror("Unexpected connect() #1 success");
+		exit(EXIT_FAILURE);
+	}
+
+	/* Vulnerable system may crash now. */
+	if (!vsock_connect_fd(fd, VMADDR_CID_HOST, VMADDR_PORT_ANY)) {
+		perror("Unexpected connect() #2 success");
+		exit(EXIT_FAILURE);
+	}
+
+	close(fd);
+	while (i--)
+		close(sockets[i]);
+
+	control_writeln("DONE");
+}
+
+static void test_stream_transport_uaf_server(const struct test_opts *opts)
+{
+	control_expectln("DONE");
+}
+
 static struct test_case test_cases[] = {
 	{
 		.name = "SOCK_STREAM connection reset",
@@ -1588,6 +1641,11 @@ static struct test_case test_cases[] = {
 		.run_client = test_seqpacket_unsent_bytes_client,
 		.run_server = test_seqpacket_unsent_bytes_server,
 	},
+	{
+		.name = "SOCK_STREAM transport release use-after-free",
+		.run_client = test_stream_transport_uaf_client,
+		.run_server = test_stream_transport_uaf_server,
+	},
 	{},
 };
 

-- 
2.48.1

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

[Michal Luczaj]

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 <mhal@rbox.co>
---
 tools/testing/vsock/vsock_test.c | 47 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index 572e0fd3e5a841f846fb304a24192f63d57ec052..5cac08d909fe495aec5ddc9f3779432f9e0dc2b8 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -1511,6 +1511,48 @@ static void test_stream_transport_uaf_server(const struct test_opts *opts)
 	control_expectln("DONE");
 }
 
+static void test_stream_connect_retry_client(const struct test_opts *opts)
+{
+	int fd;
+
+	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
+	if (fd < 0) {
+		perror("socket");
+		exit(EXIT_FAILURE);
+	}
+
+	if (!vsock_connect_fd(fd, opts->peer_cid, opts->peer_port)) {
+		fprintf(stderr, "Unexpected connect() #1 success\n");
+		exit(EXIT_FAILURE);
+	}
+
+	control_writeln("LISTEN");
+	control_expectln("LISTENING");
+
+	if (vsock_connect_fd(fd, opts->peer_cid, opts->peer_port)) {
+		perror("connect() #2");
+		exit(EXIT_FAILURE);
+	}
+
+	close(fd);
+}
+
+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",
@@ -1646,6 +1688,11 @@ static struct test_case test_cases[] = {
 		.run_client = test_stream_transport_uaf_client,
 		.run_server = test_stream_transport_uaf_server,
 	},
+	{
+		.name = "SOCK_STREAM retry failed connect()",
+		.run_client = test_stream_connect_retry_client,
+		.run_server = test_stream_connect_retry_server,
+	},
 	{},
 };
 

-- 
2.48.1

Re: [PATCH net v2 4/6] vsock/test: Introduce vsock_connect_fd()

[Stefano Garzarella]

On Tue, Jan 21, 2025 at 03:44:05PM +0100, Michal Luczaj wrote:
>Distill timeout-guarded vsock_connect_fd(). Adapt callers.
>
>Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> tools/testing/vsock/util.c | 45 +++++++++++++++++----------------------------
> tools/testing/vsock/util.h |  1 +
> 2 files changed, 18 insertions(+), 28 deletions(-)

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

>
>diff --git a/tools/testing/vsock/util.c b/tools/testing/vsock/util.c
>index 31ee1767c8b73c05cfd219c3d520a677df6e66a6..7f7e45a6596c19b09176ea2851a490cdac0f115b 100644
>--- a/tools/testing/vsock/util.c
>+++ b/tools/testing/vsock/util.c
>@@ -119,27 +119,33 @@ int vsock_bind(unsigned int cid, unsigned int port, int type)
> 	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)
>+int vsock_connect_fd(int fd, unsigned int cid, unsigned int port)
> {
>-	struct sockaddr_vm sa_server = {
>+	struct sockaddr_vm sa = {
> 		.svm_family = AF_VSOCK,
> 		.svm_cid = cid,
> 		.svm_port = port,
> 	};
>-
>-	int client_fd, ret;
>-
>-	client_fd = vsock_bind(VMADDR_CID_ANY, bind_port, type);
>+	int ret;
>
> 	timeout_begin(TIMEOUT);
> 	do {
>-		ret = connect(client_fd, (struct sockaddr *)&sa_server, sizeof(sa_server));
>+		ret = connect(fd, (struct sockaddr *)&sa, sizeof(sa));
> 		timeout_check("connect");
> 	} while (ret < 0 && errno == EINTR);
> 	timeout_end();
>
>-	if (ret < 0) {
>+	return ret;
>+}
>+
>+/* 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 client_fd;
>+
>+	client_fd = vsock_bind(VMADDR_CID_ANY, bind_port, type);
>+
>+	if (vsock_connect_fd(client_fd, cid, port)) {
> 		perror("connect");
> 		exit(EXIT_FAILURE);
> 	}
>@@ -150,17 +156,6 @@ int vsock_bind_connect(unsigned int cid, unsigned int port, unsigned int bind_po
> /* Connect to <cid, port> and return the file descriptor. */
> int vsock_connect(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 ret;
> 	int fd;
>
> 	control_expectln("LISTENING");
>@@ -171,20 +166,14 @@ int vsock_connect(unsigned int cid, unsigned int port, int type)
> 		exit(EXIT_FAILURE);
> 	}
>
>-	timeout_begin(TIMEOUT);
>-	do {
>-		ret = connect(fd, &addr.sa, sizeof(addr.svm));
>-		timeout_check("connect");
>-	} while (ret < 0 && errno == EINTR);
>-	timeout_end();
>-
>-	if (ret < 0) {
>+	if (vsock_connect_fd(fd, cid, port)) {
> 		int old_errno = errno;
>
> 		close(fd);
> 		fd = -1;
> 		errno = old_errno;
> 	}
>+
> 	return fd;
> }
>
>diff --git a/tools/testing/vsock/util.h b/tools/testing/vsock/util.h
>index 7736594a15d29449d98bd1e9e19c3acd1a623443..817e11e483cd6596dd32d16061d801a66091c973 100644
>--- a/tools/testing/vsock/util.h
>+++ b/tools/testing/vsock/util.h
>@@ -39,6 +39,7 @@ struct test_case {
> void init_signals(void);
> unsigned int parse_cid(const char *str);
> unsigned int parse_port(const char *str);
>+int vsock_connect_fd(int fd, unsigned int cid, unsigned int port);
> 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);
>
>-- 
>2.48.1
>

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

[Stefano Garzarella]

On Tue, Jan 21, 2025 at 03:44:06PM +0100, Michal Luczaj wrote:
>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 <mhal@rbox.co>
>---
> tools/testing/vsock/vsock_test.c | 58 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 58 insertions(+)

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

>
>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>index 28a5083bbfd600cf84a1a85cec2f272ce6912dd3..572e0fd3e5a841f846fb304a24192f63d57ec052 100644
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -1458,6 +1458,59 @@ 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 */
>+
>+/* Test attempts to trigger a transport release for an unbound socket. This can
>+ * lead to a reference count mishandling.
>+ */
>+static void test_stream_transport_uaf_client(const struct test_opts *opts)
>+{
>+	int sockets[MAX_PORT_RETRIES];
>+	struct sockaddr_vm addr;
>+	int fd, i, alen;
>+
>+	fd = vsock_bind(VMADDR_CID_ANY, VMADDR_PORT_ANY, SOCK_STREAM);
>+
>+	alen = sizeof(addr);
>+	if (getsockname(fd, (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_STREAM);
>+
>+	close(fd);
>+	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
>+	if (fd < 0) {
>+		perror("socket");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	if (!vsock_connect_fd(fd, addr.svm_cid, addr.svm_port)) {
>+		perror("Unexpected connect() #1 success");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	/* Vulnerable system may crash now. */
>+	if (!vsock_connect_fd(fd, VMADDR_CID_HOST, VMADDR_PORT_ANY)) {
>+		perror("Unexpected connect() #2 success");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	close(fd);
>+	while (i--)
>+		close(sockets[i]);
>+
>+	control_writeln("DONE");
>+}
>+
>+static void test_stream_transport_uaf_server(const struct test_opts *opts)
>+{
>+	control_expectln("DONE");
>+}
>+
> static struct test_case test_cases[] = {
> 	{
> 		.name = "SOCK_STREAM connection reset",
>@@ -1588,6 +1641,11 @@ static struct test_case test_cases[] = {
> 		.run_client = test_seqpacket_unsent_bytes_client,
> 		.run_server = test_seqpacket_unsent_bytes_server,
> 	},
>+	{
>+		.name = "SOCK_STREAM transport release use-after-free",
>+		.run_client = test_stream_transport_uaf_client,
>+		.run_server = test_stream_transport_uaf_server,
>+	},
> 	{},
> };
>
>
>-- 
>2.48.1
>

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

[Stefano Garzarella]

On Tue, Jan 21, 2025 at 03:44:07PM +0100, Michal Luczaj wrote:
>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 <mhal@rbox.co>
>---
> tools/testing/vsock/vsock_test.c | 47 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)

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

>
>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>index 572e0fd3e5a841f846fb304a24192f63d57ec052..5cac08d909fe495aec5ddc9f3779432f9e0dc2b8 100644
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -1511,6 +1511,48 @@ static void test_stream_transport_uaf_server(const struct test_opts *opts)
> 	control_expectln("DONE");
> }
>
>+static void test_stream_connect_retry_client(const struct test_opts *opts)
>+{
>+	int fd;
>+
>+	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
>+	if (fd < 0) {
>+		perror("socket");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	if (!vsock_connect_fd(fd, opts->peer_cid, opts->peer_port)) {
>+		fprintf(stderr, "Unexpected connect() #1 success\n");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	control_writeln("LISTEN");
>+	control_expectln("LISTENING");
>+
>+	if (vsock_connect_fd(fd, opts->peer_cid, opts->peer_port)) {
>+		perror("connect() #2");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	close(fd);
>+}
>+
>+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",
>@@ -1646,6 +1688,11 @@ static struct test_case test_cases[] = {
> 		.run_client = test_stream_transport_uaf_client,
> 		.run_server = test_stream_transport_uaf_server,
> 	},
>+	{
>+		.name = "SOCK_STREAM retry failed connect()",
>+		.run_client = test_stream_connect_retry_client,
>+		.run_server = test_stream_connect_retry_server,
>+	},
> 	{},
> };
>
>
>-- 
>2.48.1
>

Re: [PATCH net v2 0/6] vsock: Transport reassignment and error handling issues

[Stefano Garzarella]

On Tue, Jan 21, 2025 at 03:44:01PM +0100, Michal Luczaj wrote:
>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 <mhal@rbox.co>
>---
>Changes in v2:
>- Introduce vsock_connect_fd(), simplify the tests, stick to SOCK_STREAM,
>  collect Reviewed-by (Stefano)
>- Link to v1: https://lore.kernel.org/r/20250117-vsock-transport-vs-autobind-v1-0-c802c803762d@rbox.co

Thanks for sorting out my comments, I've reviewed it all and got it
running, it seems to be going well!

Stefano

>
>---
>Michal Luczaj (6):
>      vsock: Keep the binding until socket destruction
>      vsock: Allow retrying on connect() failure
>      vsock/test: Introduce vsock_bind()
>      vsock/test: Introduce vsock_connect_fd()
>      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       |  87 +++++++++++-----------------
> tools/testing/vsock/util.h       |   2 +
> tools/testing/vsock/vsock_test.c | 122 ++++++++++++++++++++++++++++++++++-----
> 4 files changed, 152 insertions(+), 72 deletions(-)
>---
>base-commit: d640627663bfe7d8963c7615316d7d4ef60f3b0b
>change-id: 20250116-vsock-transport-vs-autobind-2da49f1d5a0a
>
>Best regards,
>-- 
>Michal Luczaj <mhal@rbox.co>
>

Re: [PATCH net v2 0/6] vsock: Transport reassignment and error handling issues

[Michal Luczaj]

On 1/22/25 12:45, Stefano Garzarella wrote:
> On Tue, Jan 21, 2025 at 03:44:01PM +0100, Michal Luczaj wrote:
>> 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 <mhal@rbox.co>
>> ---
>> Changes in v2:
>> - Introduce vsock_connect_fd(), simplify the tests, stick to SOCK_STREAM,
>>  collect Reviewed-by (Stefano)
>> - Link to v1: https://lore.kernel.org/r/20250117-vsock-transport-vs-autobind-v1-0-c802c803762d@rbox.co
> 
> Thanks for sorting out my comments, I've reviewed it all and got it
> running, it seems to be going well!

Great! I was worried that I might have oversimplified the UAF selftest
(won't trigger the splat if second transport == NULL), so please let me
know if it starts acting strangely (quietly passes the test on an unpatched
system), and for what combination of enabled transports.

Thanks,
Michal

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

[Luigi Leonardi]

On Tue, Jan 21, 2025 at 03:44:07PM +0100, Michal Luczaj wrote:
>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 <mhal@rbox.co>
>---
> tools/testing/vsock/vsock_test.c | 47 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)
>
>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>index 572e0fd3e5a841f846fb304a24192f63d57ec052..5cac08d909fe495aec5ddc9f3779432f9e0dc2b8 100644
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -1511,6 +1511,48 @@ static void test_stream_transport_uaf_server(const struct test_opts *opts)
> 	control_expectln("DONE");
> }
>
>+static void test_stream_connect_retry_client(const struct test_opts *opts)
>+{
>+	int fd;
>+
>+	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
>+	if (fd < 0) {
>+		perror("socket");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	if (!vsock_connect_fd(fd, opts->peer_cid, opts->peer_port)) {
>+		fprintf(stderr, "Unexpected connect() #1 success\n");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	control_writeln("LISTEN");
>+	control_expectln("LISTENING");
>+
>+	if (vsock_connect_fd(fd, opts->peer_cid, opts->peer_port)) {
>+		perror("connect() #2");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	close(fd);
>+}
>+
>+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",
>@@ -1646,6 +1688,11 @@ static struct test_case test_cases[] = {
> 		.run_client = test_stream_transport_uaf_client,
> 		.run_server = test_stream_transport_uaf_server,
> 	},
>+	{
>+		.name = "SOCK_STREAM retry failed connect()",
>+		.run_client = test_stream_connect_retry_client,
>+		.run_server = test_stream_connect_retry_server,
>+	},
> 	{},
> };
>
>
>-- 
>2.48.1
>

LGTM!

Reviewed-by: Luigi Leonardi <leonardi@redhat.com>

Re: [PATCH net v2 0/6] vsock: Transport reassignment and error handling issues

[Stefano Garzarella]

On Wed, 22 Jan 2025 at 15:16, Michal Luczaj <mhal@rbox.co> wrote:
>
> On 1/22/25 12:45, Stefano Garzarella wrote:
> > On Tue, Jan 21, 2025 at 03:44:01PM +0100, Michal Luczaj wrote:
> >> 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 <mhal@rbox.co>
> >> ---
> >> Changes in v2:
> >> - Introduce vsock_connect_fd(), simplify the tests, stick to SOCK_STREAM,
> >>  collect Reviewed-by (Stefano)
> >> - Link to v1: https://lore.kernel.org/r/20250117-vsock-transport-vs-autobind-v1-0-c802c803762d@rbox.co
> >
> > Thanks for sorting out my comments, I've reviewed it all and got it
> > running, it seems to be going well!
>
> Great! I was worried that I might have oversimplified the UAF selftest
> (won't trigger the splat if second transport == NULL), so please let me
> know if it starts acting strangely (quietly passes the test on an unpatched
> system), and for what combination of enabled transports.

Yeah, I was worrying the same and thinking if it's better to add more
connect also with LOOPBACK and a CID > 2 to be sure we test all the
scenarios, but we can do later, for now let's have this series merged
to fix the real issue.

I tested without the fixes (first 2 patches) and I can see the
use-after-free reports only on the "host" where I have both loopback
and H2G loaded, but this should be fine.

Thanks for your work!
Stefano

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

[Luigi Leonardi]

On Tue, Jan 21, 2025 at 03:44:04PM +0100, Michal Luczaj wrote:
>Add a helper for socket()+bind(). Adapt callers.
>
>Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> 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)
If you need to send a v3, it would be nice to have a comment for 
vsock_bind, as there used to be one.
>+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.48.1
>

Thanks for the patch, I left small a comment, but if no v3 is needed 
then:

Reviewed-by: Luigi Leonardi <leonardi@redhat.com>

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

[Luigi Leonardi]

On Tue, Jan 21, 2025 at 03:44:03PM +0100, Michal Luczaj wrote:
>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")
>Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> 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;
Just to understand: Why do you reset sk_error after calling to 
transport->connect and not before?

My worry is that a transport might check this field and return an error.
IIUC with virtio-based transports this is not the case.
>+
> 		/* Mark sock as connecting and set the error code to in
> 		 * progress in case this is a non-blocking connect.
> 		 */
>
>-- 
>2.48.1
>

Thanks,
Luigi

Re: [PATCH net v2 0/6] vsock: Transport reassignment and error handling issues

[Michal Luczaj]

On 1/22/25 16:47, Stefano Garzarella wrote:
> On Wed, 22 Jan 2025 at 15:16, Michal Luczaj <mhal@rbox.co> wrote:
>>
>> On 1/22/25 12:45, Stefano Garzarella wrote:
>>> On Tue, Jan 21, 2025 at 03:44:01PM +0100, Michal Luczaj wrote:
>>>> 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 <mhal@rbox.co>
>>>> ---
>>>> Changes in v2:
>>>> - Introduce vsock_connect_fd(), simplify the tests, stick to SOCK_STREAM,
>>>>  collect Reviewed-by (Stefano)
>>>> - Link to v1: https://lore.kernel.org/r/20250117-vsock-transport-vs-autobind-v1-0-c802c803762d@rbox.co
>>>
>>> Thanks for sorting out my comments, I've reviewed it all and got it
>>> running, it seems to be going well!
>>
>> Great! I was worried that I might have oversimplified the UAF selftest
>> (won't trigger the splat if second transport == NULL), so please let me
>> know if it starts acting strangely (quietly passes the test on an unpatched
>> system), and for what combination of enabled transports.
> 
> Yeah, I was worrying the same and thinking if it's better to add more
> connect also with LOOPBACK and a CID > 2 to be sure we test all the
> scenarios, but we can do later, for now let's have this series merged
> to fix the real issue.

Sure, I'll take care of this CID galore later on.

> I tested without the fixes (first 2 patches) and I can see the
> use-after-free reports only on the "host" where I have both loopback
> and H2G loaded, but this should be fine.

Argh, sorry. FWIW, re-adding a bind() after the second connect should
increase the coverage.

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

[Michal Luczaj]

On 1/22/25 17:01, Luigi Leonardi wrote:
> On Tue, Jan 21, 2025 at 03:44:04PM +0100, Michal Luczaj wrote:
>> Add a helper for socket()+bind(). Adapt callers.
>>
>> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>> ---
>> 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)
>
> If you need to send a v3, it would be nice to have a comment for 
> vsock_bind, as there used to be one.

Comment for vsock_bind_connect() remains, see below. As for vsock_bind(),
perhaps it's time to start using kernel-doc comments? v3 isn't coming, it
seems, but I'll comment the function later.

Thanks,
Michal

>> +/* 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));

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

[Michal Luczaj]

On 1/22/25 17:28, Luigi Leonardi wrote:
> On Tue, Jan 21, 2025 at 03:44:03PM +0100, Michal Luczaj wrote:
>> 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")
>> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>> ---
>> 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;
>
> Just to understand: Why do you reset sk_error after calling to 
> transport->connect and not before?

transport->connect() can fail. In such case, I thought, it would be better
to keep the old value of sk_err. Otherwise we'd have an early failing
vsock_connect() that clears sk_err.

> My worry is that a transport might check this field and return an error.
> IIUC with virtio-based transports this is not the case.

Right, transport might check, but currently none of the transports do.

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

[Luigi Leonardi]

On Wed, Jan 22, 2025 at 09:11:30PM +0100, Michal Luczaj wrote:
>On 1/22/25 17:01, Luigi Leonardi wrote:
>> On Tue, Jan 21, 2025 at 03:44:04PM +0100, Michal Luczaj wrote:
>>> Add a helper for socket()+bind(). Adapt callers.
>>>
>>> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>> ---
>>> 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)
>>
>> If you need to send a v3, it would be nice to have a comment for
>> vsock_bind, as there used to be one.
>
>Comment for vsock_bind_connect() remains, see below. As for vsock_bind(),
>perhaps it's time to start using kernel-doc comments? 
This is a good idea! @Stefano WDYT?
Sticking to tests, IIRC someone mentioned (maybe Stefano?) about moving 
to selftests for vsock, but I don't think it's going to happen anytime 
soon.

>v3 isn't coming, it seems, but I'll comment the function later.
Thank you :)

Cheers,
Luigi

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

[Stefano Garzarella]

On Thu, Jan 23, 2025 at 12:02:36PM +0100, Luigi Leonardi wrote:
>On Wed, Jan 22, 2025 at 09:11:30PM +0100, Michal Luczaj wrote:
>>On 1/22/25 17:01, Luigi Leonardi wrote:
>>>On Tue, Jan 21, 2025 at 03:44:04PM +0100, Michal Luczaj wrote:
>>>>Add a helper for socket()+bind(). Adapt callers.
>>>>
>>>>Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>>>>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>>>---
>>>>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)
>>>
>>>If you need to send a v3, it would be nice to have a comment for
>>>vsock_bind, as there used to be one.
>>
>>Comment for vsock_bind_connect() remains, see below. As for vsock_bind(),
>>perhaps it's time to start using kernel-doc comments?
>This is a good idea! @Stefano WDYT?

I'm not sure it's worth it since they are just tests, but if someone
wants to do it, absolutely no objection.

Thanks,
Stefano

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

[Luigi Leonardi]

On Wed, Jan 22, 2025 at 10:06:51PM +0100, Michal Luczaj wrote:
>On 1/22/25 17:28, Luigi Leonardi wrote:
>> On Tue, Jan 21, 2025 at 03:44:03PM +0100, Michal Luczaj wrote:
>>> 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")
>>> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>> ---
>>> 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;
>>
>> Just to understand: Why do you reset sk_error after calling to
>> transport->connect and not before?
>
>transport->connect() can fail. In such case, I thought, it would be better
>to keep the old value of sk_err. Otherwise we'd have an early failing
>vsock_connect() that clears sk_err.
That's a good point, transport->connect doesn't set sk_err if it fails.
Thanks for the clarification :)

Reviewed-by: Luigi Leonardi <leonardi@redhat.com>
>
>> My worry is that a transport might check this field and return an error.
>> IIUC with virtio-based transports this is not the case.
>
>Right, transport might check, but currently none of the transports do.
>

Re: [PATCH net v2 0/6] vsock: Transport reassignment and error handling issues

[Jakub Kicinski]

On Tue, 21 Jan 2025 15:44:01 +0100 Michal Luczaj wrote:
> 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.

Looks like the merge window merges made this series no longer apply.
Could you rebase & repost?
-- 
pw-bot: cr

Re: [PATCH net v2 0/6] vsock: Transport reassignment and error handling issues

[Michal Luczaj]

On 1/27/25 22:49, Jakub Kicinski wrote:
> On Tue, 21 Jan 2025 15:44:01 +0100 Michal Luczaj wrote:
>> 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.
> 
> Looks like the merge window merges made this series no longer apply.
> Could you rebase & repost?

Sure, there it is:
https://lore.kernel.org/netdev/20250128-vsock-transport-vs-autobind-v3-0-1cf57065b770@rbox.co/