linux-kernel - 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: <gfmoupl72tjyymhwxcstwpgaabbfaz6f4v6vj4lwwzwssg577c@urkmgn7rapnj>
Date: Mon, 26 May 2025 16:39:55 +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 net-next] vsock/test: Cover more CIDs in transport_uaf
 test

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