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: <skvayogoenhntikkdnqrkkjvqesmpnukjlil6reubrouo45sat@j7zw6lfthfrd>
Date: Tue, 27 May 2025 10:41:05 +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 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");
> 	}
> }
>