[PATCH RFC net-next v2 0/3] vsock/test: Improve transport_uaf test

[PATCH RFC net-next v2 0/3] vsock/test: Improve transport_uaf test

[Michal Luczaj]

Increase the coverage of a test implemented in commit 301a62dfb0d0
("vsock/test: Add test for UAF due to socket unbinding"). Take this
opportunity to factor out some utility code, drop a redundant sync between
client and server, and introduce a /proc/kallsyms harvesting logic.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
Changes in v2:
- Speed up: don't bother checking EINTR or respecting timeout on connect()s
- Introduce get_transports(), warn on unsupported setup [Stefano]
- Comment the code, drop the sync, introduce vsock_bind_try() [Stefano]
- Link to v1: https://lore.kernel.org/r/20250523-vsock-test-inc-cov-v1-1-fa3507941bbd@rbox.co

---
Michal Luczaj (3):
      vsock/test: Introduce vsock_bind_try() helper
      vsock/test: Introduce get_transports()
      vsock/test: Cover more CIDs in transport_uaf test

 tools/testing/vsock/util.c       | 84 ++++++++++++++++++++++++++++++++++++++--
 tools/testing/vsock/util.h       | 13 +++++++
 tools/testing/vsock/vsock_test.c | 83 ++++++++++++++++++++++++++++++---------
 3 files changed, 158 insertions(+), 22 deletions(-)
---
base-commit: 7e34abd434644fdc3f7efe02a7f0b7947cd06aac
change-id: 20250326-vsock-test-inc-cov-b823822bdb78

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

[PATCH RFC net-next v2 1/3] vsock/test: Introduce vsock_bind_try() helper

[Michal Luczaj]

Create a socket and bind() it. If binding failed, gracefully return an
error code while preserving `errno`.

Base vsock_bind() on top of it.

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

diff --git a/tools/testing/vsock/util.c b/tools/testing/vsock/util.c
index 0c7e9cbcbc85cde9c8764fc3bb623cde2f6c77a6..b7b3fb2221c1682ecde58cf12e2f0b0ded1cff39 100644
--- a/tools/testing/vsock/util.c
+++ b/tools/testing/vsock/util.c
@@ -121,15 +121,17 @@ bool vsock_wait_sent(int fd)
 	return !ret;
 }
 
-/* Create socket <type>, bind to <cid, port> and return the file descriptor. */
-int vsock_bind(unsigned int cid, unsigned int port, int type)
+/* Create socket <type>, bind to <cid, port>.
+ * Return the file descriptor, or -1 on error.
+ */
+int vsock_bind_try(unsigned int cid, unsigned int port, int type)
 {
 	struct sockaddr_vm sa = {
 		.svm_family = AF_VSOCK,
 		.svm_cid = cid,
 		.svm_port = port,
 	};
-	int fd;
+	int fd, saved_errno;
 
 	fd = socket(AF_VSOCK, type, 0);
 	if (fd < 0) {
@@ -138,6 +140,22 @@ int vsock_bind(unsigned int cid, unsigned int port, int type)
 	}
 
 	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa))) {
+		saved_errno = errno;
+		close(fd);
+		errno = saved_errno;
+		fd = -1;
+	}
+
+	return fd;
+}
+
+/* Create socket <type>, bind to <cid, port> and return the file descriptor. */
+int vsock_bind(unsigned int cid, unsigned int port, int type)
+{
+	int fd;
+
+	fd = vsock_bind_try(cid, port, type);
+	if (fd < 0) {
 		perror("bind");
 		exit(EXIT_FAILURE);
 	}
diff --git a/tools/testing/vsock/util.h b/tools/testing/vsock/util.h
index 5e2db67072d5053804a9bb93934b625ea78bcd7a..0afe7cbae12e5194172c639ccfbeb8b81f7c25ac 100644
--- a/tools/testing/vsock/util.h
+++ b/tools/testing/vsock/util.h
@@ -44,6 +44,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_try(unsigned int cid, unsigned int port, int type);
 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);

-- 
2.49.0

[PATCH RFC net-next v2 2/3] vsock/test: Introduce get_transports()

[Michal Luczaj]

Return a bitmap of registered vsock transports. As guesstimated by grepping
/proc/kallsyms (CONFIG_KALLSYMS=y) for known symbols of type `struct
virtio_transport`.

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

diff --git a/tools/testing/vsock/util.c b/tools/testing/vsock/util.c
index b7b3fb2221c1682ecde58cf12e2f0b0ded1cff39..74fb52f566148b16436251216dd9d9275f0ec95b 100644
--- a/tools/testing/vsock/util.c
+++ b/tools/testing/vsock/util.c
@@ -7,6 +7,7 @@
  * Author: Stefan Hajnoczi <stefanha@redhat.com>
  */
 
+#include <ctype.h>
 #include <errno.h>
 #include <stdio.h>
 #include <stdint.h>
@@ -17,6 +18,7 @@
 #include <assert.h>
 #include <sys/epoll.h>
 #include <sys/mman.h>
+#include <linux/kernel.h>
 #include <linux/sockios.h>
 
 #include "timeout.h"
@@ -854,3 +856,61 @@ void enable_so_linger(int fd, int timeout)
 		exit(EXIT_FAILURE);
 	}
 }
+
+static int __get_transports(void)
+{
+	/* Order must match transports defined in util.h.
+	 * man nm: "d" The symbol is in the initialized data section.
+	 */
+	const char * const syms[] = {
+		"d loopback_transport",
+		"d virtio_transport",
+		"d vhost_transport",
+		"d vmci_transport",
+		"d hvs_transport",
+	};
+	char buf[KALLSYMS_LINE_LEN];
+	int ret = 0;
+	FILE *f;
+
+	f = fopen(KALLSYMS_PATH, "r");
+	if (!f) {
+		perror("Can't open " KALLSYMS_PATH);
+		exit(EXIT_FAILURE);
+	}
+
+	while (fgets(buf, sizeof(buf), f)) {
+		char *match;
+		int i;
+
+		assert(buf[strlen(buf) - 1] == '\n');
+
+		for (i = 0; i < ARRAY_SIZE(syms); ++i) {
+			match = strstr(buf, syms[i]);
+
+			/* Match should be followed by '\t' or '\n'.
+			 * See kallsyms.c:s_show().
+			 */
+			if (match && isspace(match[strlen(syms[i])])) {
+				ret |= _BITUL(i);
+				break;
+			}
+		}
+	}
+
+	fclose(f);
+	return ret;
+}
+
+/* Return integer with TRANSPORT_* bit set for every (known) registered vsock
+ * transport.
+ */
+int get_transports(void)
+{
+	static int tr = -1;
+
+	if (tr == -1)
+		tr = __get_transports();
+
+	return tr;
+}
diff --git a/tools/testing/vsock/util.h b/tools/testing/vsock/util.h
index 0afe7cbae12e5194172c639ccfbeb8b81f7c25ac..63953e32c3e18e1aa5c2addcf6f09f433660fa84 100644
--- a/tools/testing/vsock/util.h
+++ b/tools/testing/vsock/util.h
@@ -3,8 +3,19 @@
 #define UTIL_H
 
 #include <sys/socket.h>
+#include <linux/bitops.h>
 #include <linux/vm_sockets.h>
 
+#define KALLSYMS_PATH		"/proc/kallsyms"
+#define KALLSYMS_LINE_LEN	512
+
+/* All known transports */
+#define TRANSPORT_LOOPBACK	_BITUL(0)
+#define TRANSPORT_VIRTIO	_BITUL(1)
+#define TRANSPORT_VHOST		_BITUL(2)
+#define TRANSPORT_VMCI		_BITUL(3)
+#define TRANSPORT_HYPERV	_BITUL(4)
+
 /* Tests can either run as the client or the server */
 enum test_mode {
 	TEST_MODE_UNSET,
@@ -82,4 +93,5 @@ void setsockopt_timeval_check(int fd, int level, int optname,
 			      struct timeval val, char const *errmsg);
 void enable_so_zerocopy_check(int fd);
 void enable_so_linger(int fd, int timeout);
+int get_transports(void);
 #endif /* UTIL_H */

-- 
2.49.0

[PATCH RFC net-next v2 3/3] vsock/test: Cover more CIDs in transport_uaf test

[Michal Luczaj]

Increase the coverage of test for UAF due to socket unbinding, and losing
transport in general. It's a follow up to commit 301a62dfb0d0 ("vsock/test:
Add test for UAF due to socket unbinding") and discussion in [1].

The idea remains the same: take an unconnected stream socket with a
transport assigned and then attempt to switch the transport by trying (and
failing) to connect to some other CID. Now do this iterating over all the
well known CIDs (plus one).

Note that having only a virtio transport loaded (without vhost_vsock) is
unsupported; test will always pass. Depending on transports available, a
variety of splats are possible on unpatched machines. After reverting
commit 78dafe1cf3af ("vsock: Orphan socket after transport release") and
commit fcdd2242c023 ("vsock: Keep the binding until socket destruction"):

BUG: KASAN: slab-use-after-free in __vsock_bind+0x61f/0x720
Read of size 4 at addr ffff88811ff46b54 by task vsock_test/1475
Call Trace:
 dump_stack_lvl+0x68/0x90
 print_report+0x170/0x53d
 kasan_report+0xc2/0x180
 __vsock_bind+0x61f/0x720
 vsock_connect+0x727/0xc40
 __sys_connect+0xe8/0x100
 __x64_sys_connect+0x6e/0xc0
 do_syscall_64+0x92/0x1c0
 entry_SYSCALL_64_after_hwframe+0x4b/0x53

WARNING: CPU: 0 PID: 1475 at net/vmw_vsock/virtio_transport_common.c:37 virtio_transport_send_pkt_info+0xb2b/0x1160
Call Trace:
 virtio_transport_connect+0x90/0xb0
 vsock_connect+0x782/0xc40
 __sys_connect+0xe8/0x100
 __x64_sys_connect+0x6e/0xc0
 do_syscall_64+0x92/0x1c0
 entry_SYSCALL_64_after_hwframe+0x4b/0x53

KASAN: null-ptr-deref in range [0x0000000000000010-0x0000000000000017]
RIP: 0010:sock_has_perm+0xa7/0x2a0
Call Trace:
 selinux_socket_connect_helper.isra.0+0xbc/0x450
 selinux_socket_connect+0x3b/0x70
 security_socket_connect+0x31/0xd0
 __sys_connect_file+0x79/0x1f0
 __sys_connect+0xe8/0x100
 __x64_sys_connect+0x6e/0xc0
 do_syscall_64+0x92/0x1c0
 entry_SYSCALL_64_after_hwframe+0x4b/0x53

refcount_t: addition on 0; use-after-free.
WARNING: CPU: 7 PID: 1518 at lib/refcount.c:25 refcount_warn_saturate+0xdd/0x140
RIP: 0010:refcount_warn_saturate+0xdd/0x140
Call Trace:
 __vsock_bind+0x65e/0x720
 vsock_connect+0x727/0xc40
 __sys_connect+0xe8/0x100
 __x64_sys_connect+0x6e/0xc0
 do_syscall_64+0x92/0x1c0
 entry_SYSCALL_64_after_hwframe+0x4b/0x53

refcount_t: underflow; use-after-free.
WARNING: CPU: 0 PID: 1475 at lib/refcount.c:28 refcount_warn_saturate+0x12b/0x140
RIP: 0010:refcount_warn_saturate+0x12b/0x140
Call Trace:
 vsock_remove_bound+0x18f/0x280
 __vsock_release+0x371/0x480
 vsock_release+0x88/0x120
 __sock_release+0xaa/0x260
 sock_close+0x14/0x20
 __fput+0x35a/0xaa0
 task_work_run+0xff/0x1c0
 do_exit+0x849/0x24c0
 make_task_dead+0xf3/0x110
 rewind_stack_and_make_dead+0x16/0x20

[1]: https://lore.kernel.org/netdev/CAGxU2F5zhfWymY8u0hrKksW8PumXAYz-9_qRmW==92oAx1BX3g@mail.gmail.com/

Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 tools/testing/vsock/vsock_test.c | 83 +++++++++++++++++++++++++++++++---------
 1 file changed, 64 insertions(+), 19 deletions(-)

diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index f669baaa0dca3bebc678d00eafa80857d1f0fdd6..b58736023981ef7c4812e069ea577fcf2c0fe9fa 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -1718,16 +1718,27 @@ static void test_stream_msgzcopy_leak_zcskb_server(const struct test_opts *opts)
 
 #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)
+static bool test_stream_transport_uaf(int cid)
 {
 	int sockets[MAX_PORT_RETRIES];
 	struct sockaddr_vm addr;
-	int fd, i, alen;
+	socklen_t alen;
+	int fd, i, c;
+	bool ret;
+
+	/* Probe for a transport by attempting a local CID bind. Unavailable
+	 * transport (or more specifically: an unsupported transport/CID
+	 * combination) results in EADDRNOTAVAIL, other errnos are fatal.
+	 */
+	fd = vsock_bind_try(cid, VMADDR_PORT_ANY, SOCK_STREAM);
+	if (fd < 0) {
+		if (errno != EADDRNOTAVAIL) {
+			perror("Unexpected bind() errno");
+			exit(EXIT_FAILURE);
+		}
 
-	fd = vsock_bind(VMADDR_CID_ANY, VMADDR_PORT_ANY, SOCK_STREAM);
+		return false;
+	}
 
 	alen = sizeof(addr);
 	if (getsockname(fd, (struct sockaddr *)&addr, &alen)) {
@@ -1735,38 +1746,73 @@ static void test_stream_transport_uaf_client(const struct test_opts *opts)
 		exit(EXIT_FAILURE);
 	}
 
+	/* Drain the autobind pool; see __vsock_bind_connectible(). */
 	for (i = 0; i < MAX_PORT_RETRIES; ++i)
-		sockets[i] = vsock_bind(VMADDR_CID_ANY, ++addr.svm_port,
-					SOCK_STREAM);
+		sockets[i] = vsock_bind(cid, ++addr.svm_port, SOCK_STREAM);
 
 	close(fd);
-	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
+	fd = socket(AF_VSOCK, SOCK_STREAM | SOCK_NONBLOCK, 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");
+	/* Assign transport, while failing to autobind. Autobind pool was
+	 * drained, so EADDRNOTAVAIL coming from __vsock_bind_connectible() is
+	 * expected.
+	 */
+	addr.svm_port = VMADDR_PORT_ANY;
+	if (!connect(fd, (struct sockaddr *)&addr, alen)) {
+		fprintf(stderr, "Unexpected connect() success\n");
+		exit(EXIT_FAILURE);
+	} else if (errno == ENODEV) {
+		/* Handle unhappy vhost_vsock */
+		ret = false;
+		goto cleanup;
+	} else if (errno != EADDRNOTAVAIL) {
+		perror("Unexpected connect() errno");
 		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);
+	/* Reassign transport, triggering old transport release and
+	 * (potentially) unbinding of an unbound socket.
+	 *
+	 * Vulnerable system may crash now.
+	 */
+	for (c = VMADDR_CID_HYPERVISOR; c <= VMADDR_CID_HOST + 1; ++c) {
+		if (c != cid) {
+			addr.svm_cid = c;
+			(void)connect(fd, (struct sockaddr *)&addr, alen);
+		}
 	}
 
+	ret = true;
+cleanup:
 	close(fd);
 	while (i--)
 		close(sockets[i]);
 
-	control_writeln("DONE");
+	return ret;
 }
 
-static void test_stream_transport_uaf_server(const struct test_opts *opts)
+/* 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)
 {
-	control_expectln("DONE");
+	bool tested = false;
+	int cid, tr;
+
+	for (cid = VMADDR_CID_HYPERVISOR; cid <= VMADDR_CID_HOST + 1; ++cid)
+		tested |= test_stream_transport_uaf(cid);
+
+	tr = get_transports();
+	if (!tr)
+		fprintf(stderr, "No transports detected\n");
+	else if (tr == TRANSPORT_VIRTIO)
+		fprintf(stderr, "Setup unsupported: sole virtio transport\n");
+	else if (!tested)
+		fprintf(stderr, "No transports tested\n");
 }
 
 static void test_stream_connect_retry_client(const struct test_opts *opts)
@@ -2034,7 +2080,6 @@ static struct test_case test_cases[] = {
 	{
 		.name = "SOCK_STREAM transport release use-after-free",
 		.run_client = test_stream_transport_uaf_client,
-		.run_server = test_stream_transport_uaf_server,
 	},
 	{
 		.name = "SOCK_STREAM retry failed connect()",

-- 
2.49.0

Re: [PATCH RFC net-next v2 2/3] vsock/test: Introduce get_transports()

[Stefano Garzarella]

On Wed, May 28, 2025 at 10:44:42PM +0200, Michal Luczaj wrote:
>Return a bitmap of registered vsock transports. As guesstimated by grepping
>/proc/kallsyms (CONFIG_KALLSYMS=y) for known symbols of type `struct
>virtio_transport`.
>
>Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> tools/testing/vsock/util.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++
> tools/testing/vsock/util.h | 12 ++++++++++
> 2 files changed, 72 insertions(+)
>
>diff --git a/tools/testing/vsock/util.c b/tools/testing/vsock/util.c
>index b7b3fb2221c1682ecde58cf12e2f0b0ded1cff39..74fb52f566148b16436251216dd9d9275f0ec95b 100644
>--- a/tools/testing/vsock/util.c
>+++ b/tools/testing/vsock/util.c
>@@ -7,6 +7,7 @@
>  * Author: Stefan Hajnoczi <stefanha@redhat.com>
>  */
>
>+#include <ctype.h>
> #include <errno.h>
> #include <stdio.h>
> #include <stdint.h>
>@@ -17,6 +18,7 @@
> #include <assert.h>
> #include <sys/epoll.h>
> #include <sys/mman.h>
>+#include <linux/kernel.h>
> #include <linux/sockios.h>
>
> #include "timeout.h"
>@@ -854,3 +856,61 @@ void enable_so_linger(int fd, int timeout)
> 		exit(EXIT_FAILURE);
> 	}
> }
>+
>+static int __get_transports(void)
>+{
>+	/* Order must match transports defined in util.h.
>+	 * man nm: "d" The symbol is in the initialized data section.
>+	 */
>+	const char * const syms[] = {
>+		"d loopback_transport",
>+		"d virtio_transport",
>+		"d vhost_transport",
>+		"d vmci_transport",
>+		"d hvs_transport",
>+	};

I would move this array (or a macro that define it), near the transport 
defined in util.h, so they are near and we can easily update/review 
changes.

BTW what about adding static asserts to check we are aligned?

>+	char buf[KALLSYMS_LINE_LEN];
>+	int ret = 0;
>+	FILE *f;
>+
>+	f = fopen(KALLSYMS_PATH, "r");
>+	if (!f) {
>+		perror("Can't open " KALLSYMS_PATH);
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	while (fgets(buf, sizeof(buf), f)) {
>+		char *match;
>+		int i;
>+
>+		assert(buf[strlen(buf) - 1] == '\n');
>+
>+		for (i = 0; i < ARRAY_SIZE(syms); ++i) {
>+			match = strstr(buf, syms[i]);
>+
>+			/* Match should be followed by '\t' or '\n'.
>+			 * See kallsyms.c:s_show().
>+			 */
>+			if (match && isspace(match[strlen(syms[i])])) {
>+				ret |= _BITUL(i);
>+				break;
>+			}
>+		}
>+	}
>+
>+	fclose(f);
>+	return ret;
>+}
>+
>+/* Return integer with TRANSPORT_* bit set for every (known) registered vsock
>+ * transport.
>+ */
>+int get_transports(void)
>+{
>+	static int tr = -1;
>+
>+	if (tr == -1)
>+		tr = __get_transports();
>+
>+	return tr;
>+}
>diff --git a/tools/testing/vsock/util.h b/tools/testing/vsock/util.h
>index 0afe7cbae12e5194172c639ccfbeb8b81f7c25ac..63953e32c3e18e1aa5c2addcf6f09f433660fa84 100644
>--- a/tools/testing/vsock/util.h
>+++ b/tools/testing/vsock/util.h
>@@ -3,8 +3,19 @@
> #define UTIL_H
>
> #include <sys/socket.h>
>+#include <linux/bitops.h>
> #include <linux/vm_sockets.h>
>
>+#define KALLSYMS_PATH		"/proc/kallsyms"
>+#define KALLSYMS_LINE_LEN	512

We don't need to expose them in util.h IMO, we can keep in util.c

>+
>+/* All known transports */
>+#define TRANSPORT_LOOPBACK	_BITUL(0)
>+#define TRANSPORT_VIRTIO	_BITUL(1)
>+#define TRANSPORT_VHOST		_BITUL(2)
>+#define TRANSPORT_VMCI		_BITUL(3)
>+#define TRANSPORT_HYPERV	_BITUL(4)
>+
> /* Tests can either run as the client or the server */
> enum test_mode {
> 	TEST_MODE_UNSET,
>@@ -82,4 +93,5 @@ void setsockopt_timeval_check(int fd, int level, int optname,
> 			      struct timeval val, char const *errmsg);
> void enable_so_zerocopy_check(int fd);
> void enable_so_linger(int fd, int timeout);
>+int get_transports(void);
> #endif /* UTIL_H */
>
>-- 
>2.49.0
>

Re: [PATCH RFC net-next v2 1/3] vsock/test: Introduce vsock_bind_try() helper

[Stefano Garzarella]

On Wed, May 28, 2025 at 10:44:41PM +0200, Michal Luczaj wrote:
>Create a socket and bind() it. If binding failed, gracefully return an
>error code while preserving `errno`.
>
>Base vsock_bind() on top of it.
>
>Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> tools/testing/vsock/util.c | 24 +++++++++++++++++++++---
> tools/testing/vsock/util.h |  1 +
> 2 files changed, 22 insertions(+), 3 deletions(-)

LGTM!

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

>
>diff --git a/tools/testing/vsock/util.c b/tools/testing/vsock/util.c
>index 0c7e9cbcbc85cde9c8764fc3bb623cde2f6c77a6..b7b3fb2221c1682ecde58cf12e2f0b0ded1cff39 100644
>--- a/tools/testing/vsock/util.c
>+++ b/tools/testing/vsock/util.c
>@@ -121,15 +121,17 @@ bool vsock_wait_sent(int fd)
> 	return !ret;
> }
>
>-/* Create socket <type>, bind to <cid, port> and return the file descriptor. */
>-int vsock_bind(unsigned int cid, unsigned int port, int type)
>+/* Create socket <type>, bind to <cid, port>.
>+ * Return the file descriptor, or -1 on error.
>+ */
>+int vsock_bind_try(unsigned int cid, unsigned int port, int type)
> {
> 	struct sockaddr_vm sa = {
> 		.svm_family = AF_VSOCK,
> 		.svm_cid = cid,
> 		.svm_port = port,
> 	};
>-	int fd;
>+	int fd, saved_errno;
>
> 	fd = socket(AF_VSOCK, type, 0);
> 	if (fd < 0) {
>@@ -138,6 +140,22 @@ int vsock_bind(unsigned int cid, unsigned int port, int type)
> 	}
>
> 	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa))) {
>+		saved_errno = errno;
>+		close(fd);
>+		errno = saved_errno;
>+		fd = -1;
>+	}
>+
>+	return fd;
>+}
>+
>+/* Create socket <type>, bind to <cid, port> and return the file descriptor. */
>+int vsock_bind(unsigned int cid, unsigned int port, int type)
>+{
>+	int fd;
>+
>+	fd = vsock_bind_try(cid, port, type);
>+	if (fd < 0) {
> 		perror("bind");
> 		exit(EXIT_FAILURE);
> 	}
>diff --git a/tools/testing/vsock/util.h b/tools/testing/vsock/util.h
>index 5e2db67072d5053804a9bb93934b625ea78bcd7a..0afe7cbae12e5194172c639ccfbeb8b81f7c25ac 100644
>--- a/tools/testing/vsock/util.h
>+++ b/tools/testing/vsock/util.h
>@@ -44,6 +44,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_try(unsigned int cid, unsigned int port, int type);
> 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);
>
>-- 
>2.49.0
>

Re: [PATCH RFC net-next v2 3/3] vsock/test: Cover more CIDs in transport_uaf test

[Stefano Garzarella]

On Wed, May 28, 2025 at 10:44:43PM +0200, Michal Luczaj wrote:
>Increase the coverage of test for UAF due to socket unbinding, and losing
>transport in general. It's a follow up to commit 301a62dfb0d0 ("vsock/test:
>Add test for UAF due to socket unbinding") and discussion in [1].
>
>The idea remains the same: take an unconnected stream socket with a
>transport assigned and then attempt to switch the transport by trying (and
>failing) to connect to some other CID. Now do this iterating over all the
>well known CIDs (plus one).
>
>Note that having only a virtio transport loaded (without vhost_vsock) is
>unsupported; test will always pass. Depending on transports available, a
>variety of splats are possible on unpatched machines. After reverting
>commit 78dafe1cf3af ("vsock: Orphan socket after transport release") and
>commit fcdd2242c023 ("vsock: Keep the binding until socket destruction"):
>
>BUG: KASAN: slab-use-after-free in __vsock_bind+0x61f/0x720
>Read of size 4 at addr ffff88811ff46b54 by task vsock_test/1475
>Call Trace:
> dump_stack_lvl+0x68/0x90
> print_report+0x170/0x53d
> kasan_report+0xc2/0x180
> __vsock_bind+0x61f/0x720
> vsock_connect+0x727/0xc40
> __sys_connect+0xe8/0x100
> __x64_sys_connect+0x6e/0xc0
> do_syscall_64+0x92/0x1c0
> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>
>WARNING: CPU: 0 PID: 1475 at net/vmw_vsock/virtio_transport_common.c:37 virtio_transport_send_pkt_info+0xb2b/0x1160
>Call Trace:
> virtio_transport_connect+0x90/0xb0
> vsock_connect+0x782/0xc40
> __sys_connect+0xe8/0x100
> __x64_sys_connect+0x6e/0xc0
> do_syscall_64+0x92/0x1c0
> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>
>KASAN: null-ptr-deref in range [0x0000000000000010-0x0000000000000017]
>RIP: 0010:sock_has_perm+0xa7/0x2a0
>Call Trace:
> selinux_socket_connect_helper.isra.0+0xbc/0x450
> selinux_socket_connect+0x3b/0x70
> security_socket_connect+0x31/0xd0
> __sys_connect_file+0x79/0x1f0
> __sys_connect+0xe8/0x100
> __x64_sys_connect+0x6e/0xc0
> do_syscall_64+0x92/0x1c0
> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>
>refcount_t: addition on 0; use-after-free.
>WARNING: CPU: 7 PID: 1518 at lib/refcount.c:25 refcount_warn_saturate+0xdd/0x140
>RIP: 0010:refcount_warn_saturate+0xdd/0x140
>Call Trace:
> __vsock_bind+0x65e/0x720
> vsock_connect+0x727/0xc40
> __sys_connect+0xe8/0x100
> __x64_sys_connect+0x6e/0xc0
> do_syscall_64+0x92/0x1c0
> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>
>refcount_t: underflow; use-after-free.
>WARNING: CPU: 0 PID: 1475 at lib/refcount.c:28 refcount_warn_saturate+0x12b/0x140
>RIP: 0010:refcount_warn_saturate+0x12b/0x140
>Call Trace:
> vsock_remove_bound+0x18f/0x280
> __vsock_release+0x371/0x480
> vsock_release+0x88/0x120
> __sock_release+0xaa/0x260
> sock_close+0x14/0x20
> __fput+0x35a/0xaa0
> task_work_run+0xff/0x1c0
> do_exit+0x849/0x24c0
> make_task_dead+0xf3/0x110
> rewind_stack_and_make_dead+0x16/0x20
>
>[1]: https://lore.kernel.org/netdev/CAGxU2F5zhfWymY8u0hrKksW8PumXAYz-9_qRmW==92oAx1BX3g@mail.gmail.com/
>
>Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> tools/testing/vsock/vsock_test.c | 83 +++++++++++++++++++++++++++++++---------
> 1 file changed, 64 insertions(+), 19 deletions(-)
>
>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>index f669baaa0dca3bebc678d00eafa80857d1f0fdd6..b58736023981ef7c4812e069ea577fcf2c0fe9fa 100644
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -1718,16 +1718,27 @@ static void test_stream_msgzcopy_leak_zcskb_server(const struct test_opts *opts)
>
> #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)
>+static bool test_stream_transport_uaf(int cid)
> {
> 	int sockets[MAX_PORT_RETRIES];
> 	struct sockaddr_vm addr;
>-	int fd, i, alen;
>+	socklen_t alen;
>+	int fd, i, c;
>+	bool ret;
>+
>+	/* Probe for a transport by attempting a local CID bind. Unavailable
>+	 * transport (or more specifically: an unsupported transport/CID
>+	 * combination) results in EADDRNOTAVAIL, other errnos are fatal.
>+	 */
>+	fd = vsock_bind_try(cid, VMADDR_PORT_ANY, SOCK_STREAM);
>+	if (fd < 0) {
>+		if (errno != EADDRNOTAVAIL) {
>+			perror("Unexpected bind() errno");
>+			exit(EXIT_FAILURE);
>+		}
>
>-	fd = vsock_bind(VMADDR_CID_ANY, VMADDR_PORT_ANY, SOCK_STREAM);
>+		return false;
>+	}
>
> 	alen = sizeof(addr);
> 	if (getsockname(fd, (struct sockaddr *)&addr, &alen)) {
>@@ -1735,38 +1746,73 @@ static void test_stream_transport_uaf_client(const struct test_opts *opts)
> 		exit(EXIT_FAILURE);
> 	}
>
>+	/* Drain the autobind pool; see __vsock_bind_connectible(). */
> 	for (i = 0; i < MAX_PORT_RETRIES; ++i)
>-		sockets[i] = vsock_bind(VMADDR_CID_ANY, ++addr.svm_port,
>-					SOCK_STREAM);
>+		sockets[i] = vsock_bind(cid, ++addr.svm_port, SOCK_STREAM);
>
> 	close(fd);
>-	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
>+	fd = socket(AF_VSOCK, SOCK_STREAM | SOCK_NONBLOCK, 0);

Why we need this change?

> 	if (fd < 0) {
> 		perror("socket");
> 		exit(EXIT_FAILURE);
> 	}
>
>-	if (!vsock_connect_fd(fd, addr.svm_cid, addr.svm_port)) {
>-		perror("Unexpected connect() #1 success");
>+	/* Assign transport, while failing to autobind. Autobind pool was
>+	 * drained, so EADDRNOTAVAIL coming from __vsock_bind_connectible() is
>+	 * expected.
>+	 */
>+	addr.svm_port = VMADDR_PORT_ANY;
>+	if (!connect(fd, (struct sockaddr *)&addr, alen)) {
>+		fprintf(stderr, "Unexpected connect() success\n");
>+		exit(EXIT_FAILURE);
>+	} else if (errno == ENODEV) {
>+		/* Handle unhappy vhost_vsock */

Why it's unhappy? No peer?

>+		ret = false;
>+		goto cleanup;
>+	} else if (errno != EADDRNOTAVAIL) {
>+		perror("Unexpected connect() errno");
> 		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);
>+	/* Reassign transport, triggering old transport release and
>+	 * (potentially) unbinding of an unbound socket.
>+	 *
>+	 * Vulnerable system may crash now.
>+	 */
>+	for (c = VMADDR_CID_HYPERVISOR; c <= VMADDR_CID_HOST + 1; ++c) {
>+		if (c != cid) {
>+			addr.svm_cid = c;
>+			(void)connect(fd, (struct sockaddr *)&addr, alen);
>+		}
> 	}
>
>+	ret = true;
>+cleanup:
> 	close(fd);
> 	while (i--)
> 		close(sockets[i]);
>
>-	control_writeln("DONE");
>+	return ret;
> }
>
>-static void test_stream_transport_uaf_server(const struct test_opts *opts)
>+/* 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)
> {
>-	control_expectln("DONE");
>+	bool tested = false;
>+	int cid, tr;
>+
>+	for (cid = VMADDR_CID_HYPERVISOR; cid <= VMADDR_CID_HOST + 1; ++cid)
>+		tested |= test_stream_transport_uaf(cid);
>+
>+	tr = get_transports();
>+	if (!tr)
>+		fprintf(stderr, "No transports detected\n");
>+	else if (tr == TRANSPORT_VIRTIO)
>+		fprintf(stderr, "Setup unsupported: sole virtio transport\n");
>+	else if (!tested)
>+		fprintf(stderr, "No transports tested\n");
> }
>
> static void test_stream_connect_retry_client(const struct test_opts *opts)
>@@ -2034,7 +2080,6 @@ static struct test_case test_cases[] = {
> 	{
> 		.name = "SOCK_STREAM transport release use-after-free",
> 		.run_client = test_stream_transport_uaf_client,
>-		.run_server = test_stream_transport_uaf_server,

Overall LGTM. I was not able to apply, so I'll test next version.

Thanks,
Stefano

> 	},
> 	{
> 		.name = "SOCK_STREAM retry failed connect()",
>
>-- 
>2.49.0
>

Re: [PATCH RFC net-next v2 2/3] vsock/test: Introduce get_transports()

[Michal Luczaj]

On 6/4/25 11:07, Stefano Garzarella wrote:
> On Wed, May 28, 2025 at 10:44:42PM +0200, Michal Luczaj wrote:
>> +static int __get_transports(void)
>> +{
>> +	/* Order must match transports defined in util.h.
>> +	 * man nm: "d" The symbol is in the initialized data section.
>> +	 */
>> +	const char * const syms[] = {
>> +		"d loopback_transport",
>> +		"d virtio_transport",
>> +		"d vhost_transport",
>> +		"d vmci_transport",
>> +		"d hvs_transport",
>> +	};
> 
> I would move this array (or a macro that define it), near the transport 
> defined in util.h, so they are near and we can easily update/review 
> changes.
> 
> BTW what about adding static asserts to check we are aligned?

Something like

#define KNOWN_TRANSPORTS	\
	_(LOOPBACK, "loopback")	\
	_(VIRTIO, "virtio")	\
	_(VHOST, "vhost")	\
	_(VMCI, "vmci")		\
	_(HYPERV, "hvs")

enum transport {
	TRANSPORT_COUNTER_BASE = __COUNTER__ + 1,
	#define _(name, symbol)	\
		TRANSPORT_##name = _BITUL(__COUNTER__ - TRANSPORT_COUNTER_BASE),
	KNOWN_TRANSPORTS
	TRANSPORT_NUM = __COUNTER__ - TRANSPORT_COUNTER_BASE,
	#undef _
};

static char * const transport_ksyms[] = {
	#define _(name, symbol) "d " symbol "_transport",
	KNOWN_TRANSPORTS
	#undef _
};

static_assert(ARRAY_SIZE(transport_ksyms) == TRANSPORT_NUM);

?

Note that I keep pushing for naming HVS a TRANSPORT_HYPERV. Perhaps it's
better to stick to TRANSPORT_HVS after all?

>> diff --git a/tools/testing/vsock/util.h b/tools/testing/vsock/util.h
>> index 0afe7cbae12e5194172c639ccfbeb8b81f7c25ac..63953e32c3e18e1aa5c2addcf6f09f433660fa84 100644
>> --- a/tools/testing/vsock/util.h
>> +++ b/tools/testing/vsock/util.h
>> @@ -3,8 +3,19 @@
>> #define UTIL_H
>>
>> #include <sys/socket.h>
>> +#include <linux/bitops.h>
>> #include <linux/vm_sockets.h>
>>
>> +#define KALLSYMS_PATH		"/proc/kallsyms"
>> +#define KALLSYMS_LINE_LEN	512
> 
> We don't need to expose them in util.h IMO, we can keep in util.c

OK, sure.

Thanks,
Michal

Re: [PATCH RFC net-next v2 3/3] vsock/test: Cover more CIDs in transport_uaf test

[Michal Luczaj]

On 6/4/25 11:37, Stefano Garzarella wrote:
> On Wed, May 28, 2025 at 10:44:43PM +0200, Michal Luczaj wrote:
>> +static bool test_stream_transport_uaf(int cid)
>> {
>> 	int sockets[MAX_PORT_RETRIES];
>> 	struct sockaddr_vm addr;
>> -	int fd, i, alen;
>> +	socklen_t alen;
>> +	int fd, i, c;
>> +	bool ret;
>> +
>> +	/* Probe for a transport by attempting a local CID bind. Unavailable
>> +	 * transport (or more specifically: an unsupported transport/CID
>> +	 * combination) results in EADDRNOTAVAIL, other errnos are fatal.
>> +	 */
>> +	fd = vsock_bind_try(cid, VMADDR_PORT_ANY, SOCK_STREAM);
>> +	if (fd < 0) {
>> +		if (errno != EADDRNOTAVAIL) {
>> +			perror("Unexpected bind() errno");
>> +			exit(EXIT_FAILURE);
>> +		}
>>
>> -	fd = vsock_bind(VMADDR_CID_ANY, VMADDR_PORT_ANY, SOCK_STREAM);
>> +		return false;
>> +	}
>>
>> 	alen = sizeof(addr);
>> 	if (getsockname(fd, (struct sockaddr *)&addr, &alen)) {
>> @@ -1735,38 +1746,73 @@ static void test_stream_transport_uaf_client(const struct test_opts *opts)
>> 		exit(EXIT_FAILURE);
>> 	}
>>
>> +	/* Drain the autobind pool; see __vsock_bind_connectible(). */
>> 	for (i = 0; i < MAX_PORT_RETRIES; ++i)
>> -		sockets[i] = vsock_bind(VMADDR_CID_ANY, ++addr.svm_port,
>> -					SOCK_STREAM);
>> +		sockets[i] = vsock_bind(cid, ++addr.svm_port, SOCK_STREAM);
>>
>> 	close(fd);
>> -	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
>> +	fd = socket(AF_VSOCK, SOCK_STREAM | SOCK_NONBLOCK, 0);
> 
> Why we need this change?

It's for the (void)connect() below (not the connect() expecting early
EADDRNOTAVAIL, the second one). We're not connecting to anything anyway, so
there's no point entering the main `while (sk->sk_state != TCP_ESTABLISHED`
loop in vsock_connect().

>> 	if (fd < 0) {
>> 		perror("socket");
>> 		exit(EXIT_FAILURE);
>> 	}
>>
>> -	if (!vsock_connect_fd(fd, addr.svm_cid, addr.svm_port)) {
>> -		perror("Unexpected connect() #1 success");
>> +	/* Assign transport, while failing to autobind. Autobind pool was
>> +	 * drained, so EADDRNOTAVAIL coming from __vsock_bind_connectible() is
>> +	 * expected.
>> +	 */
>> +	addr.svm_port = VMADDR_PORT_ANY;

(Ugh, this line looks useless...)

>> +	if (!connect(fd, (struct sockaddr *)&addr, alen)) {
>> +		fprintf(stderr, "Unexpected connect() success\n");
>> +		exit(EXIT_FAILURE);
>> +	} else if (errno == ENODEV) {
>> +		/* Handle unhappy vhost_vsock */
> 
> Why it's unhappy? No peer?

It's the case of test_stream_transport_uaf(VMADDR_CID_HOST) when only
vhost_vsock transport is loaded. Before we even reach (and fail)
vsock_auto_bind(), vsock_assign_transport() fails earlier: `new_transport`
gets set to `transport_g2h` (NULL) and then it's `if (!new_transport)
return -ENODEV`. So the idea was to swallow this errno and let the caller
report that nothing went through.

I guess we can narrow this down to `if (errno == ENODEV && cid ==
VMADDR_CID_HOST)`.

>> +		ret = false;
>> +		goto cleanup;
>> +	} else if (errno != EADDRNOTAVAIL) {
>> +		perror("Unexpected connect() errno");
>> 		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);
>> +	/* Reassign transport, triggering old transport release and
>> +	 * (potentially) unbinding of an unbound socket.
>> +	 *
>> +	 * Vulnerable system may crash now.
>> +	 */
>> +	for (c = VMADDR_CID_HYPERVISOR; c <= VMADDR_CID_HOST + 1; ++c) {
>> +		if (c != cid) {
>> +			addr.svm_cid = c;
>> +			(void)connect(fd, (struct sockaddr *)&addr, alen);
>> +		}
>> 	}
>>
>> +	ret = true;
>> +cleanup:
>> 	close(fd);
>> 	while (i--)
>> 		close(sockets[i]);
>>
>> -	control_writeln("DONE");
>> +	return ret;
>> }
>>
>> -static void test_stream_transport_uaf_server(const struct test_opts *opts)
>> +/* 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)
>> {
>> -	control_expectln("DONE");
>> +	bool tested = false;
>> +	int cid, tr;
>> +
>> +	for (cid = VMADDR_CID_HYPERVISOR; cid <= VMADDR_CID_HOST + 1; ++cid)
>> +		tested |= test_stream_transport_uaf(cid);
>> +
>> +	tr = get_transports();
>> +	if (!tr)
>> +		fprintf(stderr, "No transports detected\n");
>> +	else if (tr == TRANSPORT_VIRTIO)
>> +		fprintf(stderr, "Setup unsupported: sole virtio transport\n");
>> +	else if (!tested)
>> +		fprintf(stderr, "No transports tested\n");
>> }
>>
>> static void test_stream_connect_retry_client(const struct test_opts *opts)
>> @@ -2034,7 +2080,6 @@ static struct test_case test_cases[] = {
>> 	{
>> 		.name = "SOCK_STREAM transport release use-after-free",
>> 		.run_client = test_stream_transport_uaf_client,
>> -		.run_server = test_stream_transport_uaf_server,
> 
> Overall LGTM. I was not able to apply, so I'll test next version.

Bummer, I'll make sure to rebase.

Thanks,
Michal

Re: [PATCH RFC net-next v2 2/3] vsock/test: Introduce get_transports()

[Stefano Garzarella]

On Wed, Jun 04, 2025 at 09:10:19PM +0200, Michal Luczaj wrote:
>On 6/4/25 11:07, Stefano Garzarella wrote:
>> On Wed, May 28, 2025 at 10:44:42PM +0200, Michal Luczaj wrote:
>>> +static int __get_transports(void)
>>> +{
>>> +	/* Order must match transports defined in util.h.
>>> +	 * man nm: "d" The symbol is in the initialized data section.
>>> +	 */
>>> +	const char * const syms[] = {
>>> +		"d loopback_transport",
>>> +		"d virtio_transport",
>>> +		"d vhost_transport",
>>> +		"d vmci_transport",
>>> +		"d hvs_transport",
>>> +	};
>>
>> I would move this array (or a macro that define it), near the transport
>> defined in util.h, so they are near and we can easily update/review
>> changes.
>>
>> BTW what about adding static asserts to check we are aligned?
>
>Something like
>
>#define KNOWN_TRANSPORTS	\

What about KNOWN_TRANSPORTS(_) ?

>	_(LOOPBACK, "loopback")	\
>	_(VIRTIO, "virtio")	\
>	_(VHOST, "vhost")	\
>	_(VMCI, "vmci")		\
>	_(HYPERV, "hvs")
>
>enum transport {
>	TRANSPORT_COUNTER_BASE = __COUNTER__ + 1,
>	#define _(name, symbol)	\
>		TRANSPORT_##name = _BITUL(__COUNTER__ - TRANSPORT_COUNTER_BASE),
>	KNOWN_TRANSPORTS
>	TRANSPORT_NUM = __COUNTER__ - TRANSPORT_COUNTER_BASE,
>	#undef _
>};
>
>static char * const transport_ksyms[] = {
>	#define _(name, symbol) "d " symbol "_transport",
>	KNOWN_TRANSPORTS
>	#undef _
>};
>
>static_assert(ARRAY_SIZE(transport_ksyms) == TRANSPORT_NUM);
>
>?

Yep, this is even better, thanks :-)

>
>Note that I keep pushing for naming HVS a TRANSPORT_HYPERV. Perhaps it's
>better to stick to TRANSPORT_HVS after all?

I would have used HYPERV too, but honestly I don't have a strong 
opinion, so take your choice.

Thanks,
Stefano

>
>>> diff --git a/tools/testing/vsock/util.h b/tools/testing/vsock/util.h
>>> index 0afe7cbae12e5194172c639ccfbeb8b81f7c25ac..63953e32c3e18e1aa5c2addcf6f09f433660fa84 100644
>>> --- a/tools/testing/vsock/util.h
>>> +++ b/tools/testing/vsock/util.h
>>> @@ -3,8 +3,19 @@
>>> #define UTIL_H
>>>
>>> #include <sys/socket.h>
>>> +#include <linux/bitops.h>
>>> #include <linux/vm_sockets.h>
>>>
>>> +#define KALLSYMS_PATH		"/proc/kallsyms"
>>> +#define KALLSYMS_LINE_LEN	512
>>
>> We don't need to expose them in util.h IMO, we can keep in util.c
>
>OK, sure.
>
>Thanks,
>Michal
>

Re: [PATCH RFC net-next v2 3/3] vsock/test: Cover more CIDs in transport_uaf test

[Stefano Garzarella]

On Wed, Jun 04, 2025 at 09:11:33PM +0200, Michal Luczaj wrote:
>On 6/4/25 11:37, Stefano Garzarella wrote:
>> On Wed, May 28, 2025 at 10:44:43PM +0200, Michal Luczaj wrote:
>>> +static bool test_stream_transport_uaf(int cid)
>>> {
>>> 	int sockets[MAX_PORT_RETRIES];
>>> 	struct sockaddr_vm addr;
>>> -	int fd, i, alen;
>>> +	socklen_t alen;
>>> +	int fd, i, c;
>>> +	bool ret;
>>> +
>>> +	/* Probe for a transport by attempting a local CID bind. Unavailable
>>> +	 * transport (or more specifically: an unsupported transport/CID
>>> +	 * combination) results in EADDRNOTAVAIL, other errnos are fatal.
>>> +	 */
>>> +	fd = vsock_bind_try(cid, VMADDR_PORT_ANY, SOCK_STREAM);
>>> +	if (fd < 0) {
>>> +		if (errno != EADDRNOTAVAIL) {
>>> +			perror("Unexpected bind() errno");
>>> +			exit(EXIT_FAILURE);
>>> +		}
>>>
>>> -	fd = vsock_bind(VMADDR_CID_ANY, VMADDR_PORT_ANY, SOCK_STREAM);
>>> +		return false;
>>> +	}
>>>
>>> 	alen = sizeof(addr);
>>> 	if (getsockname(fd, (struct sockaddr *)&addr, &alen)) {
>>> @@ -1735,38 +1746,73 @@ static void test_stream_transport_uaf_client(const struct test_opts *opts)
>>> 		exit(EXIT_FAILURE);
>>> 	}
>>>
>>> +	/* Drain the autobind pool; see __vsock_bind_connectible(). */
>>> 	for (i = 0; i < MAX_PORT_RETRIES; ++i)
>>> -		sockets[i] = vsock_bind(VMADDR_CID_ANY, ++addr.svm_port,
>>> -					SOCK_STREAM);
>>> +		sockets[i] = vsock_bind(cid, ++addr.svm_port, SOCK_STREAM);
>>>
>>> 	close(fd);
>>> -	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
>>> +	fd = socket(AF_VSOCK, SOCK_STREAM | SOCK_NONBLOCK, 0);
>>
>> Why we need this change?
>
>It's for the (void)connect() below (not the connect() expecting early
>EADDRNOTAVAIL, the second one). We're not connecting to anything anyway, so
>there's no point entering the main `while (sk->sk_state != TCP_ESTABLISHED`
>loop in vsock_connect().

I see now, please mention it in the commit description or a comment in 
the code (maybe better the latter).

>
>>> 	if (fd < 0) {
>>> 		perror("socket");
>>> 		exit(EXIT_FAILURE);
>>> 	}
>>>
>>> -	if (!vsock_connect_fd(fd, addr.svm_cid, addr.svm_port)) {
>>> -		perror("Unexpected connect() #1 success");
>>> +	/* Assign transport, while failing to autobind. Autobind pool was
>>> +	 * drained, so EADDRNOTAVAIL coming from __vsock_bind_connectible() is
>>> +	 * expected.
>>> +	 */
>>> +	addr.svm_port = VMADDR_PORT_ANY;
>
>(Ugh, this line looks useless...)

Yep, agree.

>
>>> +	if (!connect(fd, (struct sockaddr *)&addr, alen)) {
>>> +		fprintf(stderr, "Unexpected connect() success\n");
>>> +		exit(EXIT_FAILURE);
>>> +	} else if (errno == ENODEV) {
>>> +		/* Handle unhappy vhost_vsock */
>>
>> Why it's unhappy? No peer?
>
>It's the case of test_stream_transport_uaf(VMADDR_CID_HOST) when only
>vhost_vsock transport is loaded. Before we even reach (and fail)
>vsock_auto_bind(), vsock_assign_transport() fails earlier: `new_transport`
>gets set to `transport_g2h` (NULL) and then it's `if (!new_transport)
>return -ENODEV`. So the idea was to swallow this errno and let the caller
>report that nothing went through.
>
>I guess we can narrow this down to `if (errno == ENODEV && cid ==
>VMADDR_CID_HOST)`.

I see, yep I agree on this new idea.

>
>>> +		ret = false;
>>> +		goto cleanup;
>>> +	} else if (errno != EADDRNOTAVAIL) {
>>> +		perror("Unexpected connect() errno");
>>> 		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);
>>> +	/* Reassign transport, triggering old transport release and
>>> +	 * (potentially) unbinding of an unbound socket.
>>> +	 *
>>> +	 * Vulnerable system may crash now.
>>> +	 */
>>> +	for (c = VMADDR_CID_HYPERVISOR; c <= VMADDR_CID_HOST + 1; ++c) {
>>> +		if (c != cid) {
>>> +			addr.svm_cid = c;
>>> +			(void)connect(fd, (struct sockaddr *)&addr, alen);
>>> +		}
>>> 	}
>>>
>>> +	ret = true;
>>> +cleanup:
>>> 	close(fd);
>>> 	while (i--)
>>> 		close(sockets[i]);
>>>
>>> -	control_writeln("DONE");
>>> +	return ret;
>>> }
>>>
>>> -static void test_stream_transport_uaf_server(const struct test_opts *opts)
>>> +/* 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)
>>> {
>>> -	control_expectln("DONE");
>>> +	bool tested = false;
>>> +	int cid, tr;
>>> +
>>> +	for (cid = VMADDR_CID_HYPERVISOR; cid <= VMADDR_CID_HOST + 1; ++cid)
>>> +		tested |= test_stream_transport_uaf(cid);
>>> +
>>> +	tr = get_transports();
>>> +	if (!tr)
>>> +		fprintf(stderr, "No transports detected\n");
>>> +	else if (tr == TRANSPORT_VIRTIO)
>>> +		fprintf(stderr, "Setup unsupported: sole virtio transport\n");
>>> +	else if (!tested)
>>> +		fprintf(stderr, "No transports tested\n");
>>> }
>>>
>>> static void test_stream_connect_retry_client(const struct test_opts *opts)
>>> @@ -2034,7 +2080,6 @@ static struct test_case test_cases[] = {
>>> 	{
>>> 		.name = "SOCK_STREAM transport release use-after-free",
>>> 		.run_client = test_stream_transport_uaf_client,
>>> -		.run_server = test_stream_transport_uaf_server,
>>
>> Overall LGTM. I was not able to apply, so I'll test next version.
>
>Bummer, I'll make sure to rebase.

Thanks!
Stefano

Re: [PATCH RFC net-next v2 2/3] vsock/test: Introduce get_transports()

[Michal Luczaj]

On 6/5/25 12:46, Stefano Garzarella wrote:
> On Wed, Jun 04, 2025 at 09:10:19PM +0200, Michal Luczaj wrote:
>> On 6/4/25 11:07, Stefano Garzarella wrote:
>>> On Wed, May 28, 2025 at 10:44:42PM +0200, Michal Luczaj wrote:
>>>> +static int __get_transports(void)
>>>> +{
>>>> +	/* Order must match transports defined in util.h.
>>>> +	 * man nm: "d" The symbol is in the initialized data section.
>>>> +	 */
>>>> +	const char * const syms[] = {
>>>> +		"d loopback_transport",
>>>> +		"d virtio_transport",
>>>> +		"d vhost_transport",
>>>> +		"d vmci_transport",
>>>> +		"d hvs_transport",
>>>> +	};
>>>
>>> I would move this array (or a macro that define it), near the transport
>>> defined in util.h, so they are near and we can easily update/review
>>> changes.
>>>
>>> BTW what about adding static asserts to check we are aligned?
>>
>> Something like
>>
>> #define KNOWN_TRANSPORTS	\
> 
> What about KNOWN_TRANSPORTS(_) ?

Ah, yeah.

>> 	_(LOOPBACK, "loopback")	\
>> 	_(VIRTIO, "virtio")	\
>> 	_(VHOST, "vhost")	\
>> 	_(VMCI, "vmci")		\
>> 	_(HYPERV, "hvs")
>>
>> enum transport {
>> 	TRANSPORT_COUNTER_BASE = __COUNTER__ + 1,
>> 	#define _(name, symbol)	\
>> 		TRANSPORT_##name = _BITUL(__COUNTER__ - TRANSPORT_COUNTER_BASE),
>> 	KNOWN_TRANSPORTS
>> 	TRANSPORT_NUM = __COUNTER__ - TRANSPORT_COUNTER_BASE,
>> 	#undef _
>> };
>>
>> static char * const transport_ksyms[] = {
>> 	#define _(name, symbol) "d " symbol "_transport",
>> 	KNOWN_TRANSPORTS
>> 	#undef _
>> };
>>
>> static_assert(ARRAY_SIZE(transport_ksyms) == TRANSPORT_NUM);
>>
>> ?
> 
> Yep, this is even better, thanks :-)

Although checkpatch complains:

ERROR: Macros with complex values should be enclosed in parentheses
#105: FILE: tools/testing/vsock/util.h:11:
+#define KNOWN_TRANSPORTS(_)	\
+	_(LOOPBACK, "loopback")	\
+	_(VIRTIO, "virtio")	\
+	_(VHOST, "vhost")	\
+	_(VMCI, "vmci")		\
+	_(HYPERV, "hvs")

BUT SEE:

   do {} while (0) advice is over-stated in a few situations:

   The more obvious case is macros, like MODULE_PARM_DESC, invoked at
   file-scope, where C disallows code (it must be in functions).  See
   $exceptions if you have one to add by name.

   More troublesome is declarative macros used at top of new scope,
   like DECLARE_PER_CPU.  These might just compile with a do-while-0
   wrapper, but would be incorrect.  Most of these are handled by
   detecting struct,union,etc declaration primitives in $exceptions.

   Theres also macros called inside an if (block), which "return" an
   expression.  These cannot do-while, and need a ({}) wrapper.

   Enjoy this qualification while we work to improve our heuristics.

ERROR: Macros with complex values should be enclosed in parentheses
#114: FILE: tools/testing/vsock/util.h:20:
+	#define _(name, symbol)	\
+		TRANSPORT_##name = BIT(__COUNTER__ - TRANSPORT_COUNTER_BASE),

WARNING: Argument 'symbol' is not used in function-like macro
#114: FILE: tools/testing/vsock/util.h:20:
+	#define _(name, symbol)	\
+		TRANSPORT_##name = BIT(__COUNTER__ - TRANSPORT_COUNTER_BASE),

WARNING: Argument 'name' is not used in function-like macro
#122: FILE: tools/testing/vsock/util.h:28:
+	#define _(name, symbol) "d " symbol "_transport",

Is it ok to ignore this? FWIW, I see the same ERRORs due to similarly used
preprocessor directives in fs/bcachefs/alloc_background_format.h, and the
same WARNINGs about unused macro arguments in arch/x86/include/asm/asm.h
(e.g. __ASM_SEL).

Re: [PATCH RFC net-next v2 2/3] vsock/test: Introduce get_transports()

[Stefano Garzarella]

On Fri, Jun 06, 2025 at 09:51:29AM +0200, Michal Luczaj wrote:
>On 6/5/25 12:46, Stefano Garzarella wrote:
>> On Wed, Jun 04, 2025 at 09:10:19PM +0200, Michal Luczaj wrote:
>>> On 6/4/25 11:07, Stefano Garzarella wrote:
>>>> On Wed, May 28, 2025 at 10:44:42PM +0200, Michal Luczaj wrote:
>>>>> +static int __get_transports(void)
>>>>> +{
>>>>> +	/* Order must match transports defined in util.h.
>>>>> +	 * man nm: "d" The symbol is in the initialized data section.
>>>>> +	 */
>>>>> +	const char * const syms[] = {
>>>>> +		"d loopback_transport",
>>>>> +		"d virtio_transport",
>>>>> +		"d vhost_transport",
>>>>> +		"d vmci_transport",
>>>>> +		"d hvs_transport",
>>>>> +	};
>>>>
>>>> I would move this array (or a macro that define it), near the transport
>>>> defined in util.h, so they are near and we can easily update/review
>>>> changes.
>>>>
>>>> BTW what about adding static asserts to check we are aligned?
>>>
>>> Something like
>>>
>>> #define KNOWN_TRANSPORTS	\
>>
>> What about KNOWN_TRANSPORTS(_) ?
>
>Ah, yeah.
>
>>> 	_(LOOPBACK, "loopback")	\
>>> 	_(VIRTIO, "virtio")	\
>>> 	_(VHOST, "vhost")	\
>>> 	_(VMCI, "vmci")		\
>>> 	_(HYPERV, "hvs")
>>>
>>> enum transport {
>>> 	TRANSPORT_COUNTER_BASE = __COUNTER__ + 1,
>>> 	#define _(name, symbol)	\
>>> 		TRANSPORT_##name = _BITUL(__COUNTER__ - TRANSPORT_COUNTER_BASE),
>>> 	KNOWN_TRANSPORTS
>>> 	TRANSPORT_NUM = __COUNTER__ - TRANSPORT_COUNTER_BASE,
>>> 	#undef _
>>> };
>>>
>>> static char * const transport_ksyms[] = {
>>> 	#define _(name, symbol) "d " symbol "_transport",
>>> 	KNOWN_TRANSPORTS
>>> 	#undef _
>>> };
>>>
>>> static_assert(ARRAY_SIZE(transport_ksyms) == TRANSPORT_NUM);
>>>
>>> ?
>>
>> Yep, this is even better, thanks :-)
>
>Although checkpatch complains:
>
>ERROR: Macros with complex values should be enclosed in parentheses
>#105: FILE: tools/testing/vsock/util.h:11:
>+#define KNOWN_TRANSPORTS(_)	\
>+	_(LOOPBACK, "loopback")	\
>+	_(VIRTIO, "virtio")	\
>+	_(VHOST, "vhost")	\
>+	_(VMCI, "vmci")		\
>+	_(HYPERV, "hvs")
>
>BUT SEE:
>
>   do {} while (0) advice is over-stated in a few situations:
>
>   The more obvious case is macros, like MODULE_PARM_DESC, invoked at
>   file-scope, where C disallows code (it must be in functions).  See
>   $exceptions if you have one to add by name.
>
>   More troublesome is declarative macros used at top of new scope,
>   like DECLARE_PER_CPU.  These might just compile with a do-while-0
>   wrapper, but would be incorrect.  Most of these are handled by
>   detecting struct,union,etc declaration primitives in $exceptions.
>
>   Theres also macros called inside an if (block), which "return" an
>   expression.  These cannot do-while, and need a ({}) wrapper.
>
>   Enjoy this qualification while we work to improve our heuristics.
>
>ERROR: Macros with complex values should be enclosed in parentheses
>#114: FILE: tools/testing/vsock/util.h:20:
>+	#define _(name, symbol)	\
>+		TRANSPORT_##name = BIT(__COUNTER__ - TRANSPORT_COUNTER_BASE),
>
>WARNING: Argument 'symbol' is not used in function-like macro
>#114: FILE: tools/testing/vsock/util.h:20:
>+	#define _(name, symbol)	\
>+		TRANSPORT_##name = BIT(__COUNTER__ - TRANSPORT_COUNTER_BASE),
>
>WARNING: Argument 'name' is not used in function-like macro
>#122: FILE: tools/testing/vsock/util.h:28:
>+	#define _(name, symbol) "d " symbol "_transport",
>
>Is it ok to ignore this? FWIW, I see the same ERRORs due to similarly used
>preprocessor directives in fs/bcachefs/alloc_background_format.h, and the
>same WARNINGs about unused macro arguments in arch/x86/include/asm/asm.h
>(e.g. __ASM_SEL).

It's just test, so I think it's fine to ignore, but please exaplain it 
in the commit description with also references to other ERRORs/WARNINGs 
like you did here. Let's see what net maintainers think.

Thanks,
Stefano

Re: [PATCH RFC net-next v2 2/3] vsock/test: Introduce get_transports()

[Michal Luczaj]

On 6/11/25 16:20, Stefano Garzarella wrote:
> On Fri, Jun 06, 2025 at 09:51:29AM +0200, Michal Luczaj wrote:
>> On 6/5/25 12:46, Stefano Garzarella wrote:
>>> On Wed, Jun 04, 2025 at 09:10:19PM +0200, Michal Luczaj wrote:
>>>> On 6/4/25 11:07, Stefano Garzarella wrote:
>>>>> On Wed, May 28, 2025 at 10:44:42PM +0200, Michal Luczaj wrote:
>>>>>> +static int __get_transports(void)
>>>>>> +{
>>>>>> +	/* Order must match transports defined in util.h.
>>>>>> +	 * man nm: "d" The symbol is in the initialized data section.
>>>>>> +	 */
>>>>>> +	const char * const syms[] = {
>>>>>> +		"d loopback_transport",
>>>>>> +		"d virtio_transport",
>>>>>> +		"d vhost_transport",
>>>>>> +		"d vmci_transport",
>>>>>> +		"d hvs_transport",
>>>>>> +	};
>>>>>
>>>>> I would move this array (or a macro that define it), near the transport
>>>>> defined in util.h, so they are near and we can easily update/review
>>>>> changes.
>>>>>
>>>>> BTW what about adding static asserts to check we are aligned?
>>>>
>>>> Something like
>>>>
>>>> #define KNOWN_TRANSPORTS	\
>>>
>>> What about KNOWN_TRANSPORTS(_) ?
>>
>> Ah, yeah.
>>
>>>> 	_(LOOPBACK, "loopback")	\
>>>> 	_(VIRTIO, "virtio")	\
>>>> 	_(VHOST, "vhost")	\
>>>> 	_(VMCI, "vmci")		\
>>>> 	_(HYPERV, "hvs")
>>>>
>>>> enum transport {
>>>> 	TRANSPORT_COUNTER_BASE = __COUNTER__ + 1,
>>>> 	#define _(name, symbol)	\
>>>> 		TRANSPORT_##name = _BITUL(__COUNTER__ - TRANSPORT_COUNTER_BASE),
>>>> 	KNOWN_TRANSPORTS
>>>> 	TRANSPORT_NUM = __COUNTER__ - TRANSPORT_COUNTER_BASE,
>>>> 	#undef _
>>>> };
>>>>
>>>> static char * const transport_ksyms[] = {
>>>> 	#define _(name, symbol) "d " symbol "_transport",
>>>> 	KNOWN_TRANSPORTS
>>>> 	#undef _
>>>> };
>>>>
>>>> static_assert(ARRAY_SIZE(transport_ksyms) == TRANSPORT_NUM);
>>>>
>>>> ?
>>>
>>> Yep, this is even better, thanks :-)
>>
>> Although checkpatch complains:
>>
>> ERROR: Macros with complex values should be enclosed in parentheses
>> #105: FILE: tools/testing/vsock/util.h:11:
>> +#define KNOWN_TRANSPORTS(_)	\
>> +	_(LOOPBACK, "loopback")	\
>> +	_(VIRTIO, "virtio")	\
>> +	_(VHOST, "vhost")	\
>> +	_(VMCI, "vmci")		\
>> +	_(HYPERV, "hvs")
>>
>> BUT SEE:
>>
>>   do {} while (0) advice is over-stated in a few situations:
>>
>>   The more obvious case is macros, like MODULE_PARM_DESC, invoked at
>>   file-scope, where C disallows code (it must be in functions).  See
>>   $exceptions if you have one to add by name.
>>
>>   More troublesome is declarative macros used at top of new scope,
>>   like DECLARE_PER_CPU.  These might just compile with a do-while-0
>>   wrapper, but would be incorrect.  Most of these are handled by
>>   detecting struct,union,etc declaration primitives in $exceptions.
>>
>>   Theres also macros called inside an if (block), which "return" an
>>   expression.  These cannot do-while, and need a ({}) wrapper.
>>
>>   Enjoy this qualification while we work to improve our heuristics.
>>
>> ERROR: Macros with complex values should be enclosed in parentheses
>> #114: FILE: tools/testing/vsock/util.h:20:
>> +	#define _(name, symbol)	\
>> +		TRANSPORT_##name = BIT(__COUNTER__ - TRANSPORT_COUNTER_BASE),
>>
>> WARNING: Argument 'symbol' is not used in function-like macro
>> #114: FILE: tools/testing/vsock/util.h:20:
>> +	#define _(name, symbol)	\
>> +		TRANSPORT_##name = BIT(__COUNTER__ - TRANSPORT_COUNTER_BASE),
>>
>> WARNING: Argument 'name' is not used in function-like macro
>> #122: FILE: tools/testing/vsock/util.h:28:
>> +	#define _(name, symbol) "d " symbol "_transport",
>>
>> Is it ok to ignore this? FWIW, I see the same ERRORs due to similarly used
>> preprocessor directives in fs/bcachefs/alloc_background_format.h, and the
>> same WARNINGs about unused macro arguments in arch/x86/include/asm/asm.h
>> (e.g. __ASM_SEL).
> 
> It's just test, so I think it's fine to ignore, but please exaplain it 
> in the commit description with also references to other ERRORs/WARNINGs 
> like you did here. Let's see what net maintainers think.

Sure, I've added a note. I've also switched the magic macro name '_' to
'x', this seems to be more common.

https://lore.kernel.org/netdev/20250611-vsock-test-inc-cov-v3-0-5834060d9c20@rbox.co/

Thanks,
Michal