Re: [PATCH net 4/5] vsock/test: Add test for UAF due to socket unbinding

On 1/20/25 11:20, Stefano Garzarella wrote:

On Fri, Jan 17, 2025 at 10:59:44PM +0100, Michal Luczaj wrote:

quoted

+#define MAX_PORT_RETRIES	24	/* net/vmw_vsock/af_vsock.c */
+#define VMADDR_CID_NONEXISTING	42

I would suggest a slightly bigger and weirder CID, this might actually
exist, (e.g. 4242424242)

Ok. I'll try to avoid the whole VMADDR_CID_NONEXISTING. See below.

quoted

+/* Test attempts to trigger a transport release for an unbound socket. This can
+ * lead to a reference count mishandling.
+ */
+static void test_seqpacket_transport_uaf_client(const struct test_opts *opts)
+{
+	int sockets[MAX_PORT_RETRIES];
+	struct sockaddr_vm addr;
+	int s, i, alen;
+
+	s = vsock_bind(VMADDR_CID_LOCAL, VMADDR_PORT_ANY, SOCK_SEQPACKET);

In my suite this test failed because I have instances where I run it
without vsock_loopback loaded.

26 - connectible transport release use-after-free...bind: Cannot assign requested address

Is it important to use VMADDR_CID_LOCAL or can we use VMADDR_CID_ANY?

To force the transport release on a live socket (via transport
reassignment) we need to switch between two transports (but it's okay if
the other transport turns out to be NULL). So, considering your setup(s),
is it reasonable to expect VMADDR_CID_ANY and VMADDR_CID_HOST bringing in
different transports (as in: different pointers)? Any other combination?

I've tried that in v2:
https://lore.kernel.org/netdev/20250121-vsock-transport-vs-autobind-v2-0-aad6069a4e8c@rbox.co (local)

quoted

+	/* Vulnerable system may crash now. */
+	bind(s, (struct sockaddr *)&addr, alen);

Should we check the return of this function or do we not care whether
it fails or not?

We don't really care, but I guess it just looks bad. I'll fix that.

Thanks,
Michal