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