Message-ID: <dth7n7nahmfzbiymldaxmlir64o2ck5iqo2zldppiukfpwdiqh@u6342kxtih4c>
Date: Thu, 5 Jun 2025 12:49:33 +0200
From: Stefano Garzarella <sgarzare@...hat.com>
To: Michal Luczaj <mhal@...x.co>
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 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

Powered by blists - more mailing lists