Re: [PATCH net-next] vsock/test: Cover more CIDs in transport_uaf test

[Michal Luczaj <mhal@rbox.co>]

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?

>> 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.

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");
 	}
 }