On Mon, May 26, 2025 at 02:51:18PM +0200, Michal Luczaj wrote:

On 5/26/25 10:25, Stefano Garzarella wrote:

quoted

On Fri, May 23, 2025 at 12:31:16AM +0200, Michal Luczaj wrote:

quoted

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

quoted

quoted

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

quoted

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.

quoted

quoted

+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)

quoted

+		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