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

[PATCH net-next] 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 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 | 72 +++++++++++++++++++++++++++++++---------
 1 file changed, 57 insertions(+), 15 deletions(-)

diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index 9ea33b78b9fcb532f4f9616b38b4d2b627b04d31..460a8838e5e6a0f155e66e7720358208bab9520f 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -1729,16 +1729,32 @@ 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)
 {
+	struct sockaddr_vm addr = {
+		.svm_family = AF_VSOCK,
+		.svm_cid = cid,
+		.svm_port = VMADDR_PORT_ANY
+	};
 	int sockets[MAX_PORT_RETRIES];
-	struct sockaddr_vm addr;
-	int fd, i, alen;
+	socklen_t alen;
+	int fd, i, c;
 
-	fd = vsock_bind(VMADDR_CID_ANY, VMADDR_PORT_ANY, SOCK_STREAM);
+	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
+	if (fd < 0) {
+		perror("socket");
+		exit(EXIT_FAILURE);
+	}
+
+	if (bind(fd, (struct sockaddr *)&addr, sizeof(addr))) {
+		if (errno != EADDRNOTAVAIL) {
+			perror("Unexpected bind() errno");
+			exit(EXIT_FAILURE);
+		}
+
+		close(fd);
+		return false;
+	}
 
 	alen = sizeof(addr);
 	if (getsockname(fd, (struct sockaddr *)&addr, &alen)) {
@@ -1746,9 +1762,9 @@ 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);
@@ -1757,21 +1773,47 @@ static void test_stream_transport_uaf_client(const struct test_opts *opts)
 		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.
+	 * ENODEV indicates a missing transport.
+	 */
+	errno = 0;
+	if (!vsock_connect_fd(fd, cid, VMADDR_PORT_ANY) ||
+	    errno != EADDRNOTAVAIL) {
+		perror("Unexpected connect() result");
 		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)
+			(void)vsock_connect_fd(fd, c, VMADDR_PORT_ANY);
 	}
 
 	close(fd);
 	while (i--)
 		close(sockets[i]);
 
+	return true;
+}
+
+/* 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)
+{
+	bool tested = false;
+	int cid;
+
+	for (cid = VMADDR_CID_HYPERVISOR; cid <= VMADDR_CID_HOST + 1; ++cid)
+		tested |= test_stream_transport_uaf(cid);
+
+	if (!tested)
+		fprintf(stderr, "No transport tested\n");
+
 	control_writeln("DONE");
 }
 

---
base-commit: 610c248178b38fac2b601cd9f0f8a5e8be7fd248
change-id: 20250326-vsock-test-inc-cov-b823822bdb78

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

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

[Stefano Garzarella]

On Fri, May 23, 2025 at 12:31:16AM +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

Do you think it might make sense to print a warning if we are in this 
case, perhaps by parsing /proc/modules and looking at vsock 
dependencies?

>variety of splats are possible on unpatched machines. After reverting
>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 | 72 +++++++++++++++++++++++++++++++---------
> 1 file changed, 57 insertions(+), 15 deletions(-)
>
>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>index 9ea33b78b9fcb532f4f9616b38b4d2b627b04d31..460a8838e5e6a0f155e66e7720358208bab9520f 100644
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -1729,16 +1729,32 @@ 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)
> {
>+	struct sockaddr_vm addr = {
>+		.svm_family = AF_VSOCK,
>+		.svm_cid = cid,
>+		.svm_port = VMADDR_PORT_ANY
>+	};
> 	int sockets[MAX_PORT_RETRIES];
>-	struct sockaddr_vm addr;
>-	int fd, i, alen;
>+	socklen_t alen;
>+	int fd, i, c;
>
>-	fd = vsock_bind(VMADDR_CID_ANY, VMADDR_PORT_ANY, SOCK_STREAM);
>+	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
>+	if (fd < 0) {
>+		perror("socket");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	if (bind(fd, (struct sockaddr *)&addr, sizeof(addr))) {
>+		if (errno != EADDRNOTAVAIL) {
>+			perror("Unexpected bind() errno");
>+			exit(EXIT_FAILURE);
>+		}
>+
>+		close(fd);
>+		return false;

Perhaps we should mention in the commit or in a comment above this 
function, what we return and why we can expect EADDRNOTAVAIL.

>+	}

What about adding a vsock_bind_try() in util.c that can fail returning
errno, so we can share most of the code with vsock_bind()?

>
> 	alen = sizeof(addr);
> 	if (getsockname(fd, (struct sockaddr *)&addr, &alen)) {
>@@ -1746,9 +1762,9 @@ 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);
>@@ -1757,21 +1773,47 @@ static void test_stream_transport_uaf_client(const struct test_opts *opts)
> 		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.
>+	 * ENODEV indicates a missing transport.
>+	 */
>+	errno = 0;
>+	if (!vsock_connect_fd(fd, cid, VMADDR_PORT_ANY) ||
>+	    errno != EADDRNOTAVAIL) {
>+		perror("Unexpected connect() result");
> 		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)
>+			(void)vsock_connect_fd(fd, c, VMADDR_PORT_ANY);
> 	}
>
> 	close(fd);
> 	while (i--)
> 		close(sockets[i]);
>
>+	return true;
>+}
>+
>+/* 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)
>+{
>+	bool tested = false;
>+	int cid;
>+
>+	for (cid = VMADDR_CID_HYPERVISOR; cid <= VMADDR_CID_HOST + 1; ++cid)

>+		tested |= test_stream_transport_uaf(cid);
>+
>+	if (!tested)
>+		fprintf(stderr, "No transport tested\n");
>+
> 	control_writeln("DONE");

While we're at it, I think we can remove this message, looking at 
run_tests() in util.c, we already have a barrier.

Thanks,
Stefano

> }
>
>
>---
>base-commit: 610c248178b38fac2b601cd9f0f8a5e8be7fd248
>change-id: 20250326-vsock-test-inc-cov-b823822bdb78
>
>Best regards,
>-- 
>Michal Luczaj <mhal@rbox.co>
>

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

[Michal Luczaj]

On 5/26/25 10:25, Stefano Garzarella wrote:
> On Fri, May 23, 2025 at 12:31:16AM +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
> 
> Do you think it might make sense to print a warning if we are in this 
> case, perhaps by parsing /proc/modules and looking at vsock 
> dependencies?

That'd nice, but would parsing /proc/modules work if a transport is
compiled-in (not a module)?

>> +static bool test_stream_transport_uaf(int cid)
>> {
>> +	struct sockaddr_vm addr = {
>> +		.svm_family = AF_VSOCK,
>> +		.svm_cid = cid,
>> +		.svm_port = VMADDR_PORT_ANY
>> +	};
>> 	int sockets[MAX_PORT_RETRIES];
>> -	struct sockaddr_vm addr;
>> -	int fd, i, alen;
>> +	socklen_t alen;
>> +	int fd, i, c;
>>
>> -	fd = vsock_bind(VMADDR_CID_ANY, VMADDR_PORT_ANY, SOCK_STREAM);
>> +	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
>> +	if (fd < 0) {
>> +		perror("socket");
>> +		exit(EXIT_FAILURE);
>> +	}
>> +
>> +	if (bind(fd, (struct sockaddr *)&addr, sizeof(addr))) {
>> +		if (errno != EADDRNOTAVAIL) {
>> +			perror("Unexpected bind() errno");
>> +			exit(EXIT_FAILURE);
>> +		}
>> +
>> +		close(fd);
>> +		return false;
> 
> Perhaps we should mention in the commit or in a comment above this 
> function, what we return and why we can expect EADDRNOTAVAIL.

Something like

/* 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.
 */

?

And I've just realized feeding VMADDR_CID_HYPERVISOR to bind() doesn't make
sense at all. Will fix.

> What about adding a vsock_bind_try() in util.c that can fail returning
> errno, so we can share most of the code with vsock_bind()?

Ah, yes, good idea.

>> +static void test_stream_transport_uaf_client(const struct test_opts *opts)
>> +{
>> +	bool tested = false;
>> +	int cid;
>> +
>> +	for (cid = VMADDR_CID_HYPERVISOR; cid <= VMADDR_CID_HOST + 1; ++cid)
> 
>> +		tested |= test_stream_transport_uaf(cid);
>> +
>> +	if (!tested)
>> +		fprintf(stderr, "No transport tested\n");
>> +
>> 	control_writeln("DONE");
> 
> While we're at it, I think we can remove this message, looking at 
> run_tests() in util.c, we already have a barrier.

Ok, sure. Note that console output gets slightly de-synchronised: server
will immediately print next test's prompt and wait there.

Thanks,
Michal

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

[Stefano Garzarella]

On Mon, May 26, 2025 at 02:51:18PM +0200, Michal Luczaj wrote:
>On 5/26/25 10:25, Stefano Garzarella wrote:
>> On Fri, May 23, 2025 at 12:31:16AM +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
>>
>> Do you think it might make sense to print a warning if we are in this
>> case, perhaps by parsing /proc/modules and looking at vsock
>> dependencies?
>
>That'd nice, but would parsing /proc/modules work if a transport is
>compiled-in (not a module)?

Good point, I think not, maybe we can see something under /sys/module,
though, I would say let's do best effort without going crazy ;-)

>
>>> +static bool test_stream_transport_uaf(int cid)
>>> {
>>> +	struct sockaddr_vm addr = {
>>> +		.svm_family = AF_VSOCK,
>>> +		.svm_cid = cid,
>>> +		.svm_port = VMADDR_PORT_ANY
>>> +	};
>>> 	int sockets[MAX_PORT_RETRIES];
>>> -	struct sockaddr_vm addr;
>>> -	int fd, i, alen;
>>> +	socklen_t alen;
>>> +	int fd, i, c;
>>>
>>> -	fd = vsock_bind(VMADDR_CID_ANY, VMADDR_PORT_ANY, SOCK_STREAM);
>>> +	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
>>> +	if (fd < 0) {
>>> +		perror("socket");
>>> +		exit(EXIT_FAILURE);
>>> +	}
>>> +
>>> +	if (bind(fd, (struct sockaddr *)&addr, sizeof(addr))) {
>>> +		if (errno != EADDRNOTAVAIL) {
>>> +			perror("Unexpected bind() errno");
>>> +			exit(EXIT_FAILURE);
>>> +		}
>>> +
>>> +		close(fd);
>>> +		return false;
>>
>> Perhaps we should mention in the commit or in a comment above this
>> function, what we return and why we can expect EADDRNOTAVAIL.
>
>Something like
>
>/* 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.
> */
>
>?

LGTM!

>
>And I've just realized feeding VMADDR_CID_HYPERVISOR to bind() doesn't make
>sense at all. Will fix.

Yeah, we don't support it for now and maybe it makes sense only in the 
VMM code (e.g. QEMU), but it's a test, so if you want to leave to stress 
it more, I don't think it's a big issue.

>
>> What about adding a vsock_bind_try() in util.c that can fail returning
>> errno, so we can share most of the code with vsock_bind()?
>
>Ah, yes, good idea.
>
>>> +static void test_stream_transport_uaf_client(const struct test_opts *opts)
>>> +{
>>> +	bool tested = false;
>>> +	int cid;
>>> +
>>> +	for (cid = VMADDR_CID_HYPERVISOR; cid <= VMADDR_CID_HOST + 1; ++cid)
>>
>>> +		tested |= test_stream_transport_uaf(cid);
>>> +
>>> +	if (!tested)
>>> +		fprintf(stderr, "No transport tested\n");
>>> +
>>> 	control_writeln("DONE");
>>
>> While we're at it, I think we can remove this message, looking at
>> run_tests() in util.c, we already have a barrier.
>
>Ok, sure. Note that console output gets slightly de-synchronised: server
>will immediately print next test's prompt and wait there.

I see, however I don't have a strong opinion, you can leave it that way 
if you prefer.

Thanks,
Stefano

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

[Michal Luczaj]

On 5/26/25 16:39, Stefano Garzarella wrote:
> On Mon, May 26, 2025 at 02:51:18PM +0200, Michal Luczaj wrote:
>> On 5/26/25 10:25, Stefano Garzarella wrote:
>>> On Fri, May 23, 2025 at 12:31:16AM +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
>>>
>>> Do you think it might make sense to print a warning if we are in this
>>> case, perhaps by parsing /proc/modules and looking at vsock
>>> dependencies?
>>
>> That'd nice, but would parsing /proc/modules work if a transport is
>> compiled-in (not a module)?
> 
> Good point, I think not, maybe we can see something under /sys/module,
> though, I would say let's do best effort without going crazy ;-)

Grepping through /proc/kallsyms would do the trick. Is this still a sane
ground?

>> And I've just realized feeding VMADDR_CID_HYPERVISOR to bind() doesn't make
>> sense at all. Will fix.
> 
> Yeah, we don't support it for now and maybe it makes sense only in the 
> VMM code (e.g. QEMU), but it's a test, so if you want to leave to stress 
> it more, I don't think it's a big issue.

All right, I'll keep it then. Fails quickly and politely anyway.

>>>> +static void test_stream_transport_uaf_client(const struct test_opts *opts)
>>>> +{
>>>> +	bool tested = false;
>>>> +	int cid;
>>>> +
>>>> +	for (cid = VMADDR_CID_HYPERVISOR; cid <= VMADDR_CID_HOST + 1; ++cid)
>>>
>>>> +		tested |= test_stream_transport_uaf(cid);
>>>> +
>>>> +	if (!tested)
>>>> +		fprintf(stderr, "No transport tested\n");
>>>> +
>>>> 	control_writeln("DONE");
>>>
>>> While we're at it, I think we can remove this message, looking at
>>> run_tests() in util.c, we already have a barrier.
>>
>> Ok, sure. Note that console output gets slightly de-synchronised: server
>> will immediately print next test's prompt and wait there.
> 
> I see, however I don't have a strong opinion, you can leave it that way 
> if you prefer.

How about adding a sync point to run_tests()? E.g.

diff --git a/tools/testing/vsock/util.c b/tools/testing/vsock/util.c
index de25892f865f..79a02b52dc19 100644
--- a/tools/testing/vsock/util.c
+++ b/tools/testing/vsock/util.c
@@ -451,6 +451,9 @@ void run_tests(const struct test_case *test_cases,
 			run(opts);

 		printf("ok\n");
+
+		control_writeln("RUN_TESTS_SYNC");
+		control_expectln("RUN_TESTS_SYNC");
 	}
 }

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

[Stefano Garzarella]

On Mon, May 26, 2025 at 10:44:05PM +0200, Michal Luczaj wrote:
>On 5/26/25 16:39, Stefano Garzarella wrote:
>> On Mon, May 26, 2025 at 02:51:18PM +0200, Michal Luczaj wrote:
>>> On 5/26/25 10:25, Stefano Garzarella wrote:
>>>> On Fri, May 23, 2025 at 12:31:16AM +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
>>>>
>>>> Do you think it might make sense to print a warning if we are in this
>>>> case, perhaps by parsing /proc/modules and looking at vsock
>>>> dependencies?
>>>
>>> That'd nice, but would parsing /proc/modules work if a transport is
>>> compiled-in (not a module)?
>>
>> Good point, I think not, maybe we can see something under /sys/module,
>> though, I would say let's do best effort without going crazy ;-)
>
>Grepping through /proc/kallsyms would do the trick. Is this still a sane
>ground?

It also depends on a config right?
I see CONFIG_KALLSYMS, CONFIG_KALLSYMS_ALL, etc. but yeah, if it's 
enabled, it should work for both modules and built-in transports.

>
>>> And I've just realized feeding VMADDR_CID_HYPERVISOR to bind() doesn't make
>>> sense at all. Will fix.
>>
>> Yeah, we don't support it for now and maybe it makes sense only in the
>> VMM code (e.g. QEMU), but it's a test, so if you want to leave to stress
>> it more, I don't think it's a big issue.
>
>All right, I'll keep it then. Fails quickly and politely anyway.
>
>>>>> +static void test_stream_transport_uaf_client(const struct test_opts *opts)
>>>>> +{
>>>>> +	bool tested = false;
>>>>> +	int cid;
>>>>> +
>>>>> +	for (cid = VMADDR_CID_HYPERVISOR; cid <= VMADDR_CID_HOST + 1; ++cid)
>>>>
>>>>> +		tested |= test_stream_transport_uaf(cid);
>>>>> +
>>>>> +	if (!tested)
>>>>> +		fprintf(stderr, "No transport tested\n");
>>>>> +
>>>>> 	control_writeln("DONE");
>>>>
>>>> While we're at it, I think we can remove this message, looking at
>>>> run_tests() in util.c, we already have a barrier.
>>>
>>> Ok, sure. Note that console output gets slightly de-synchronised: server
>>> will immediately print next test's prompt and wait there.
>>
>> I see, however I don't have a strong opinion, you can leave it that way
>> if you prefer.
>
>How about adding a sync point to run_tests()? E.g.

Yep, why not, of course in another series :-)

And if you like, you can remove that specific sync point in that series 
and check also other tests, but I think we have only that one.

Thanks,
Stefano

>
>diff --git a/tools/testing/vsock/util.c b/tools/testing/vsock/util.c
>index de25892f865f..79a02b52dc19 100644
>--- a/tools/testing/vsock/util.c
>+++ b/tools/testing/vsock/util.c
>@@ -451,6 +451,9 @@ void run_tests(const struct test_case *test_cases,
> 			run(opts);
>
> 		printf("ok\n");
>+
>+		control_writeln("RUN_TESTS_SYNC");
>+		control_expectln("RUN_TESTS_SYNC");
> 	}
> }
>

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

[Michal Luczaj]

On 5/27/25 10:41, Stefano Garzarella wrote:
> On Mon, May 26, 2025 at 10:44:05PM +0200, Michal Luczaj wrote:
>> On 5/26/25 16:39, Stefano Garzarella wrote:
>>> On Mon, May 26, 2025 at 02:51:18PM +0200, Michal Luczaj wrote:
>>>> On 5/26/25 10:25, Stefano Garzarella wrote:
>>>>> On Fri, May 23, 2025 at 12:31:16AM +0200, Michal Luczaj wrote:
>>>>>> Note that having only a virtio transport loaded (without vhost_vsock) is
>>>>>> unsupported; test will always pass. Depending on transports available, a
>>>>>
>>>>> Do you think it might make sense to print a warning if we are in this
>>>>> case, perhaps by parsing /proc/modules and looking at vsock
>>>>> dependencies?
>>>>
>>>> That'd nice, but would parsing /proc/modules work if a transport is
>>>> compiled-in (not a module)?
>>>
>>> Good point, I think not, maybe we can see something under /sys/module,
>>> though, I would say let's do best effort without going crazy ;-)
>>
>> Grepping through /proc/kallsyms would do the trick. Is this still a sane
>> ground?
> 
> It also depends on a config right?
> I see CONFIG_KALLSYMS, CONFIG_KALLSYMS_ALL, etc. but yeah, if it's 
> enabled, it should work for both modules and built-in transports.

FWIW, tools/testing/selftests/net/config has CONFIG_KALLSYMS=y, which
is enough for being able to check symbols like virtio_transport and
vhost_transport.

Administrative query: while net-next is closed, am I supposed to mark this
series as "RFC" and post v2 for a review as usual, or is it better to just
hold off until net-next opens?

>>>>>> +static void test_stream_transport_uaf_client(const struct test_opts *opts)
>>>>>> +{
>>>>>> +	bool tested = false;
>>>>>> +	int cid;
>>>>>> +
>>>>>> +	for (cid = VMADDR_CID_HYPERVISOR; cid <= VMADDR_CID_HOST + 1; ++cid)
>>>>>
>>>>>> +		tested |= test_stream_transport_uaf(cid);
>>>>>> +
>>>>>> +	if (!tested)
>>>>>> +		fprintf(stderr, "No transport tested\n");
>>>>>> +
>>>>>> 	control_writeln("DONE");
>>>>>
>>>>> While we're at it, I think we can remove this message, looking at
>>>>> run_tests() in util.c, we already have a barrier.
>>>>
>>>> Ok, sure. Note that console output gets slightly de-synchronised: server
>>>> will immediately print next test's prompt and wait there.
>>>
>>> I see, however I don't have a strong opinion, you can leave it that way
>>> if you prefer.
>>
>> How about adding a sync point to run_tests()? E.g.
> 
> Yep, why not, of course in another series :-)
> 
> And if you like, you can remove that specific sync point in that series 
> and check also other tests, but I think we have only that one.

OK, I'll leave that for later.

Thanks,
Michal

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

[Stefano Garzarella]

On Wed, May 28, 2025 at 10:58:28AM +0200, Michal Luczaj wrote:
>On 5/27/25 10:41, Stefano Garzarella wrote:
>> On Mon, May 26, 2025 at 10:44:05PM +0200, Michal Luczaj wrote:
>>> On 5/26/25 16:39, Stefano Garzarella wrote:
>>>> On Mon, May 26, 2025 at 02:51:18PM +0200, Michal Luczaj wrote:
>>>>> On 5/26/25 10:25, Stefano Garzarella wrote:
>>>>>> On Fri, May 23, 2025 at 12:31:16AM +0200, Michal Luczaj wrote:
>>>>>>> Note that having only a virtio transport loaded (without vhost_vsock) is
>>>>>>> unsupported; test will always pass. Depending on transports available, a
>>>>>>
>>>>>> Do you think it might make sense to print a warning if we are in this
>>>>>> case, perhaps by parsing /proc/modules and looking at vsock
>>>>>> dependencies?
>>>>>
>>>>> That'd nice, but would parsing /proc/modules work if a transport is
>>>>> compiled-in (not a module)?
>>>>
>>>> Good point, I think not, maybe we can see something under /sys/module,
>>>> though, I would say let's do best effort without going crazy ;-)
>>>
>>> Grepping through /proc/kallsyms would do the trick. Is this still a sane
>>> ground?
>>
>> It also depends on a config right?
>> I see CONFIG_KALLSYMS, CONFIG_KALLSYMS_ALL, etc. but yeah, if it's
>> enabled, it should work for both modules and built-in transports.
>
>FWIW, tools/testing/selftests/net/config has CONFIG_KALLSYMS=y, which
>is enough for being able to check symbols like virtio_transport and
>vhost_transport.

Ok, I see, so let's go in that direction.

>
>Administrative query: while net-next is closed, am I supposed to mark this
>series as "RFC" and post v2 for a review as usual, or is it better to just
>hold off until net-next opens?

Whichever you prefer, if you are uncertain about the next version and 
want to speed things up with a review while waiting, then go with RFC, 
but if you think all comments are resolved and the next version is ready 
to be merged, wait for the reopening.
Thanks for asking!

>
>>>>>>> +static void test_stream_transport_uaf_client(const struct test_opts *opts)
>>>>>>> +{
>>>>>>> +	bool tested = false;
>>>>>>> +	int cid;
>>>>>>> +
>>>>>>> +	for (cid = VMADDR_CID_HYPERVISOR; cid <= VMADDR_CID_HOST + 1; ++cid)
>>>>>>
>>>>>>> +		tested |= test_stream_transport_uaf(cid);
>>>>>>> +
>>>>>>> +	if (!tested)
>>>>>>> +		fprintf(stderr, "No transport tested\n");
>>>>>>> +
>>>>>>> 	control_writeln("DONE");
>>>>>>
>>>>>> While we're at it, I think we can remove this message, looking at
>>>>>> run_tests() in util.c, we already have a barrier.
>>>>>
>>>>> Ok, sure. Note that console output gets slightly de-synchronised: server
>>>>> will immediately print next test's prompt and wait there.
>>>>
>>>> I see, however I don't have a strong opinion, you can leave it that way
>>>> if you prefer.
>>>
>>> How about adding a sync point to run_tests()? E.g.
>>
>> Yep, why not, of course in another series :-)
>>
>> And if you like, you can remove that specific sync point in that series
>> and check also other tests, but I think we have only that one.
>
>OK, I'll leave that for later.

Yep, feel free to discard my suggestion, we can fix it later.

Thanks,
Stefano

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

[Michal Luczaj]

On 5/28/25 11:08, Stefano Garzarella wrote:
> On Wed, May 28, 2025 at 10:58:28AM +0200, Michal Luczaj wrote:
>> Administrative query: while net-next is closed, am I supposed to mark this
>> series as "RFC" and post v2 for a review as usual, or is it better to just
>> hold off until net-next opens?
> 
> Whichever you prefer, if you are uncertain about the next version and 
> want to speed things up with a review while waiting, then go with RFC, 
> but if you think all comments are resolved and the next version is ready 
> to be merged, wait for the reopening.
> Thanks for asking!

All right then, I gave RFC a try:
https://lore.kernel.org/netdev/20250528-vsock-test-inc-cov-v2-0-8f655b40d57c@rbox.co/

>>>>>>>> +static void test_stream_transport_uaf_client(const struct test_opts *opts)
>>>>>>>> +{
>>>>>>>> +	bool tested = false;
>>>>>>>> +	int cid;
>>>>>>>> +
>>>>>>>> +	for (cid = VMADDR_CID_HYPERVISOR; cid <= VMADDR_CID_HOST + 1; ++cid)
>>>>>>>
>>>>>>>> +		tested |= test_stream_transport_uaf(cid);
>>>>>>>> +
>>>>>>>> +	if (!tested)
>>>>>>>> +		fprintf(stderr, "No transport tested\n");
>>>>>>>> +
>>>>>>>> 	control_writeln("DONE");
>>>>>>>
>>>>>>> While we're at it, I think we can remove this message, looking at
>>>>>>> run_tests() in util.c, we already have a barrier.
>>>>>>
>>>>>> Ok, sure. Note that console output gets slightly de-synchronised: server
>>>>>> will immediately print next test's prompt and wait there.
>>>>>
>>>>> I see, however I don't have a strong opinion, you can leave it that way
>>>>> if you prefer.
>>>>
>>>> How about adding a sync point to run_tests()? E.g.
>>>
>>> Yep, why not, of course in another series :-)
>>>
>>> And if you like, you can remove that specific sync point in that series
>>> and check also other tests, but I think we have only that one.
>>
>> OK, I'll leave that for later.
> 
> Yep, feel free to discard my suggestion, we can fix it later.

I was thinking about doing a console-output-beautification series
with: 1) drop the redundant sync in test_stream_transport_uaf_*, 2) add a
sync in run_tests(). But I guess we can have the sync dropping part here.
Definitely less churn this way.

Thanks,
Michal