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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <77c48b6d-4539-4d01-bd7f-7b5415b7b995@rbox.co>
Date: Wed, 4 Jun 2025 21:11:33 +0200
From: Michal Luczaj <mhal@...x.co>
To: Stefano Garzarella <sgarzare@...hat.com>
Cc: virtualization@...ts.linux.dev, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC net-next v2 3/3] vsock/test: Cover more CIDs in
 transport_uaf test

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