netdev - Re: [PATCH net-next] 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: <1f5cc46a-de4c-4361-a706-fc7fe06a7068@rbox.co>
Date: Mon, 26 May 2025 14:51:18 +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 net-next] vsock/test: Cover more CIDs in transport_uaf
 test

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