[PATCH net 0/3] vsock: Fix transport_{h2g,g2h,dgram,local} TOCTOU issues

transport_{h2g,g2h,dgram,local} may become NULL on vsock_core_unregister().
Make sure a poorly timed `rmmod transport` won't lead to a NULL/stale
pointer dereference.

Note that these oopses are pretty unlikely to happen in the wild. Splats
were collected after sprinkling kernel with mdelay()s.

Signed-off-by: Michal Luczaj <redacted>
---
Michal Luczaj (3):
      vsock: Fix transport_{h2g,g2h} TOCTOU
      vsock: Fix transport_g2h TOCTOU
      vsock: Fix transport_* TOCTOU

 net/vmw_vsock/af_vsock.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)
---
base-commit: d0fa59897e049e84432600e86df82aab3dce7aa5
change-id: 20250523-vsock-transports-toctou-4b75d9c2a805

Best regards,
-- 
Michal Luczaj [off-list ref]

Function may race with vsock_core_unregister(): transport_g2h may become
NULL after the NULL check. Protect from a potential null-ptr-deref.

KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
RIP: 0010:vsock_find_cid+0x47/0x90
Call Trace:
 __vsock_bind+0x4b2/0x720
 vsock_bind+0x90/0xe0
 __sys_bind+0x14d/0x1e0
 __x64_sys_bind+0x6e/0xc0
 do_syscall_64+0x92/0x1c0
 entry_SYSCALL_64_after_hwframe+0x4b/0x53

Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
Signed-off-by: Michal Luczaj <redacted>
---
 net/vmw_vsock/af_vsock.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 047d1bc773fab9c315a6ccd383a451fa11fb703e..337540efc237c8bc482a6730948fc773c00854f1 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c

@@ -533,8 +533,10 @@ EXPORT_SYMBOL_GPL(vsock_assign_transport);
 
 bool vsock_find_cid(unsigned int cid)
 {
-	if (transport_g2h && cid == transport_g2h->get_local_cid())
-		return true;
+	scoped_guard(mutex, &vsock_register_mutex) {
+		if (transport_g2h && cid == transport_g2h->get_local_cid())
+			return true;
+	}
 
 	if (transport_h2g && cid == VMADDR_CID_HOST)
 		return true;

-- 
2.49.0

Transport assignment may race with module unload. Protect new_transport
from becoming a stale pointer.

This also takes care of an insecure call in vsock_use_local_transport();
add a lockdep assert.

BUG: unable to handle page fault for address: fffffbfff8056000
Oops: Oops: 0000 [#1] SMP KASAN
RIP: 0010:vsock_assign_transport+0x366/0x600
Call Trace:
 vsock_connect+0x59c/0xc40
 __sys_connect+0xe8/0x100
 __x64_sys_connect+0x6e/0xc0
 do_syscall_64+0x92/0x1c0
 entry_SYSCALL_64_after_hwframe+0x4b/0x53

Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
Signed-off-by: Michal Luczaj <redacted>
---
 net/vmw_vsock/af_vsock.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 337540efc237c8bc482a6730948fc773c00854f1..133d7c8d2231e5c2e5e6a697de3b104fe05d8020 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c

@@ -407,6 +407,8 @@ EXPORT_SYMBOL_GPL(vsock_enqueue_accept);
 
 static bool vsock_use_local_transport(unsigned int remote_cid)
 {
+	lockdep_assert_held(&vsock_register_mutex);
+
 	if (!transport_local)
 		return false;
 

@@ -464,6 +466,8 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
 
 	remote_flags = vsk->remote_addr.svm_flags;
 
+	mutex_lock(&vsock_register_mutex);
+
 	switch (sk->sk_type) {
 	case SOCK_DGRAM:
 		new_transport = transport_dgram;

@@ -479,12 +483,15 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
 			new_transport = transport_h2g;
 		break;
 	default:
-		return -ESOCKTNOSUPPORT;
+		ret = -ESOCKTNOSUPPORT;
+		goto unlock;
 	}
 
 	if (vsk->transport) {
-		if (vsk->transport == new_transport)
-			return 0;
+		if (vsk->transport == new_transport) {
+			ret = 0;
+			goto unlock;
+		}
 
 		/* transport->release() must be called with sock lock acquired.
 		 * This path can only be taken during vsock_connect(), where we

@@ -508,8 +515,12 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
 	/* We increase the module refcnt to prevent the transport unloading
 	 * while there are open sockets assigned to it.
 	 */
-	if (!new_transport || !try_module_get(new_transport->module))
-		return -ENODEV;
+	if (!new_transport || !try_module_get(new_transport->module)) {
+		ret = -ENODEV;
+		goto unlock;
+	}
+
+	mutex_unlock(&vsock_register_mutex);
 
 	if (sk->sk_type == SOCK_SEQPACKET) {
 		if (!new_transport->seqpacket_allow ||

@@ -528,6 +539,9 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
 	vsk->transport = new_transport;
 
 	return 0;
+unlock:
+	mutex_unlock(&vsock_register_mutex);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(vsock_assign_transport);
 

-- 
2.49.0

Checking transport_{h2g,g2h} != NULL may race with vsock_core_unregister().
Make sure pointers remain valid.

KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
RIP: 0010:vsock_dev_do_ioctl.isra.0+0x58/0xf0
Call Trace:
 __x64_sys_ioctl+0x12d/0x190
 do_syscall_64+0x92/0x1c0
 entry_SYSCALL_64_after_hwframe+0x4b/0x53

Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
Signed-off-by: Michal Luczaj <redacted>
---
 net/vmw_vsock/af_vsock.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 2e7a3034e965db30b6ee295370d866e6d8b1c341..047d1bc773fab9c315a6ccd383a451fa11fb703e 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c

@@ -2541,6 +2541,8 @@ static long vsock_dev_do_ioctl(struct file *filp,
 
 	switch (cmd) {
 	case IOCTL_VM_SOCKETS_GET_LOCAL_CID:
+		mutex_lock(&vsock_register_mutex);
+
 		/* To be compatible with the VMCI behavior, we prioritize the
 		 * guest CID instead of well-know host CID (VMADDR_CID_HOST).
 		 */

@@ -2549,6 +2551,8 @@ static long vsock_dev_do_ioctl(struct file *filp,
 		else if (transport_h2g)
 			cid = transport_h2g->get_local_cid();
 
+		mutex_unlock(&vsock_register_mutex);
+
 		if (put_user(cid, p) != 0)
 			retval = -EFAULT;
 		break;

-- 
2.49.0

On Wed, Jun 18, 2025 at 02:34:00PM +0200, Michal Luczaj wrote:

What about if we introduce a new `vsock_get_local_cid`:

u32 vsock_get_local_cid() {
	u32 cid = VMADDR_CID_ANY;

	mutex_lock(&vsock_register_mutex);
	/* To be compatible with the VMCI behavior, we prioritize the
	 * guest CID instead of well-know host CID (VMADDR_CID_HOST).
	 */
	if (transport_g2h)
		cid = transport_g2h->get_local_cid();
	else if (transport_h2g)
		cid = transport_h2g->get_local_cid();
	mutex_lock(&vsock_register_mutex);

	return cid;
}


And we use it here, and in the place fixed by next patch?

I think we can fix all in a single patch, the problem here is to call 
transport_*->get_local_cid() without the lock IIUC.

Thanks,
Stefano

+
		if (put_user(cid, p) != 0)
			retval = -EFAULT;
		break;

-- 
2.49.0

On Wed, Jun 18, 2025 at 02:34:02PM +0200, Michal Luczaj wrote:

I'd add a comment here to explain that we can release it since we
successfully increased the `new_transport` refcnt.

I'd call it `err:` so it's clear is the error path.

Thanks,
Stefano

+	mutex_unlock(&vsock_register_mutex);
+	return ret;
}
EXPORT_SYMBOL_GPL(vsock_assign_transport);


-- 
2.49.0

On 6/20/25 10:37, Stefano Garzarella wrote:

quoted

-	if (!new_transport || !try_module_get(new_transport->module))
-		return -ENODEV;
+	if (!new_transport || !try_module_get(new_transport->module)) {
+		ret = -ENODEV;
+		goto unlock;
+	}
+

I'd add a comment here to explain that we can release it since we
successfully increased the `new_transport` refcnt.

Sure, will do.

quoted

+	mutex_unlock(&vsock_register_mutex);

	if (sk->sk_type == SOCK_SEQPACKET) {
		if (!new_transport->seqpacket_allow ||

@@ -528,6 +539,9 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)

	vsk->transport = new_transport;

	return 0;
+unlock:

I'd call it `err:` so it's clear is the error path.

Right, that makes sense.

Thanks!
Michal

On 6/20/25 10:32, Stefano Garzarella wrote:

On Wed, Jun 18, 2025 at 02:34:00PM +0200, Michal Luczaj wrote:

quoted

Checking transport_{h2g,g2h} != NULL may race with vsock_core_unregister().
Make sure pointers remain valid.

KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
RIP: 0010:vsock_dev_do_ioctl.isra.0+0x58/0xf0
Call Trace:
__x64_sys_ioctl+0x12d/0x190
do_syscall_64+0x92/0x1c0
entry_SYSCALL_64_after_hwframe+0x4b/0x53

Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
Signed-off-by: Michal Luczaj <redacted>
---
net/vmw_vsock/af_vsock.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 2e7a3034e965db30b6ee295370d866e6d8b1c341..047d1bc773fab9c315a6ccd383a451fa11fb703e 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c

@@ -2541,6 +2541,8 @@ static long vsock_dev_do_ioctl(struct file *filp,

	switch (cmd) {
	case IOCTL_VM_SOCKETS_GET_LOCAL_CID:
+		mutex_lock(&vsock_register_mutex);
+
		/* To be compatible with the VMCI behavior, we prioritize the
		 * guest CID instead of well-know host CID (VMADDR_CID_HOST).
		 */

@@ -2549,6 +2551,8 @@ static long vsock_dev_do_ioctl(struct file *filp,

		else if (transport_h2g)
			cid = transport_h2g->get_local_cid();

+		mutex_unlock(&vsock_register_mutex);

What about if we introduce a new `vsock_get_local_cid`:

u32 vsock_get_local_cid() {
	u32 cid = VMADDR_CID_ANY;

	mutex_lock(&vsock_register_mutex);
	/* To be compatible with the VMCI behavior, we prioritize the
	 * guest CID instead of well-know host CID (VMADDR_CID_HOST).
	 */
	if (transport_g2h)
		cid = transport_g2h->get_local_cid();
	else if (transport_h2g)
		cid = transport_h2g->get_local_cid();
	mutex_lock(&vsock_register_mutex);

	return cid;
}


And we use it here, and in the place fixed by next patch?

I think we can fix all in a single patch, the problem here is to call 
transport_*->get_local_cid() without the lock IIUC.

Do you mean:

 bool vsock_find_cid(unsigned int cid)
 {
-       if (transport_g2h && cid == transport_g2h->get_local_cid())
+       if (transport_g2h && cid == vsock_get_local_cid())
                return true;

?

So we need to check transport_g2h twice; in vsock_find_cid() and then again
in vsock_get_local_cid(), right?

On Fri, Jun 20, 2025 at 02:58:49PM +0200, Michal Luczaj wrote:

On 6/20/25 10:32, Stefano Garzarella wrote:

quoted

On Wed, Jun 18, 2025 at 02:34:00PM +0200, Michal Luczaj wrote:

quoted

Checking transport_{h2g,g2h} != NULL may race with vsock_core_unregister().
Make sure pointers remain valid.

KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
RIP: 0010:vsock_dev_do_ioctl.isra.0+0x58/0xf0
Call Trace:
__x64_sys_ioctl+0x12d/0x190
do_syscall_64+0x92/0x1c0
entry_SYSCALL_64_after_hwframe+0x4b/0x53

Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
Signed-off-by: Michal Luczaj <redacted>
---
net/vmw_vsock/af_vsock.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 2e7a3034e965db30b6ee295370d866e6d8b1c341..047d1bc773fab9c315a6ccd383a451fa11fb703e 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c

@@ -2541,6 +2541,8 @@ static long vsock_dev_do_ioctl(struct file *filp,

	switch (cmd) {
	case IOCTL_VM_SOCKETS_GET_LOCAL_CID:
+		mutex_lock(&vsock_register_mutex);
+
		/* To be compatible with the VMCI behavior, we prioritize the
		 * guest CID instead of well-know host CID (VMADDR_CID_HOST).
		 */

@@ -2549,6 +2551,8 @@ static long vsock_dev_do_ioctl(struct file *filp,

		else if (transport_h2g)
			cid = transport_h2g->get_local_cid();

+		mutex_unlock(&vsock_register_mutex);

What about if we introduce a new `vsock_get_local_cid`:

u32 vsock_get_local_cid() {
	u32 cid = VMADDR_CID_ANY;

	mutex_lock(&vsock_register_mutex);
	/* To be compatible with the VMCI behavior, we prioritize the
	 * guest CID instead of well-know host CID (VMADDR_CID_HOST).
	 */
	if (transport_g2h)
		cid = transport_g2h->get_local_cid();
	else if (transport_h2g)
		cid = transport_h2g->get_local_cid();
	mutex_lock(&vsock_register_mutex);

	return cid;
}


And we use it here, and in the place fixed by next patch?

I think we can fix all in a single patch, the problem here is to call
transport_*->get_local_cid() without the lock IIUC.

Do you mean:

bool vsock_find_cid(unsigned int cid)
{
-       if (transport_g2h && cid == transport_g2h->get_local_cid())
+       if (transport_g2h && cid == vsock_get_local_cid())
               return true;

?

Nope, I meant:

  bool vsock_find_cid(unsigned int cid)
  {
-       if (transport_g2h && cid == transport_g2h->get_local_cid())
-               return true;
-
-       if (transport_h2g && cid == VMADDR_CID_HOST)
+       if (cid == vsock_get_local_cid())
                 return true;

         if (transport_local && cid == VMADDR_CID_LOCAL)

But now I'm thinking if we should also include `transport_local` in the 
new `vsock_get_local_cid()`.

I think that will fix an issue when calling 
IOCTL_VM_SOCKETS_GET_LOCAL_CID and only vsock-loopback kernel module is 
loaded, so maybe we can do 2 patches:

1. fix IOCTL_VM_SOCKETS_GET_LOCAL_CID to check also `transport_local`
    Fixes: 0e12190578d0 ("vsock: add local transport support in the vsock core")

2. move that code in vsock_get_local_cid() with proper locking and use 
it also in vsock_find_cid()

WDYT?

Thanks,
Stefano

On 6/20/25 15:20, Stefano Garzarella wrote:

On Fri, Jun 20, 2025 at 02:58:49PM +0200, Michal Luczaj wrote:

quoted

On 6/20/25 10:32, Stefano Garzarella wrote:

quoted

On Wed, Jun 18, 2025 at 02:34:00PM +0200, Michal Luczaj wrote:

quoted

Checking transport_{h2g,g2h} != NULL may race with vsock_core_unregister().
Make sure pointers remain valid.

KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
RIP: 0010:vsock_dev_do_ioctl.isra.0+0x58/0xf0
Call Trace:
__x64_sys_ioctl+0x12d/0x190
do_syscall_64+0x92/0x1c0
entry_SYSCALL_64_after_hwframe+0x4b/0x53

Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
Signed-off-by: Michal Luczaj <redacted>
---
net/vmw_vsock/af_vsock.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 2e7a3034e965db30b6ee295370d866e6d8b1c341..047d1bc773fab9c315a6ccd383a451fa11fb703e 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c

@@ -2541,6 +2541,8 @@ static long vsock_dev_do_ioctl(struct file *filp,

	switch (cmd) {
	case IOCTL_VM_SOCKETS_GET_LOCAL_CID:
+		mutex_lock(&vsock_register_mutex);
+
		/* To be compatible with the VMCI behavior, we prioritize the
		 * guest CID instead of well-know host CID (VMADDR_CID_HOST).
		 */

@@ -2549,6 +2551,8 @@ static long vsock_dev_do_ioctl(struct file *filp,

		else if (transport_h2g)
			cid = transport_h2g->get_local_cid();

+		mutex_unlock(&vsock_register_mutex);

What about if we introduce a new `vsock_get_local_cid`:

u32 vsock_get_local_cid() {
	u32 cid = VMADDR_CID_ANY;

	mutex_lock(&vsock_register_mutex);
	/* To be compatible with the VMCI behavior, we prioritize the
	 * guest CID instead of well-know host CID (VMADDR_CID_HOST).
	 */
	if (transport_g2h)
		cid = transport_g2h->get_local_cid();
	else if (transport_h2g)
		cid = transport_h2g->get_local_cid();
	mutex_lock(&vsock_register_mutex);

	return cid;
}


And we use it here, and in the place fixed by next patch?

I think we can fix all in a single patch, the problem here is to call
transport_*->get_local_cid() without the lock IIUC.

Do you mean:

bool vsock_find_cid(unsigned int cid)
{
-       if (transport_g2h && cid == transport_g2h->get_local_cid())
+       if (transport_g2h && cid == vsock_get_local_cid())
               return true;

?

Nope, I meant:

  bool vsock_find_cid(unsigned int cid)
  {
-       if (transport_g2h && cid == transport_g2h->get_local_cid())
-               return true;
-
-       if (transport_h2g && cid == VMADDR_CID_HOST)
+       if (cid == vsock_get_local_cid())
                 return true;

         if (transport_local && cid == VMADDR_CID_LOCAL)

But it does change the behaviour, doesn't it? With this patch, (with g2h
loaded) if cid fails to match g2h->get_local_cid(), we don't fall back to
h2g case any more, i.e. no more comparing cid with VMADDR_CID_HOST.

But now I'm thinking if we should also include `transport_local` in the 
new `vsock_get_local_cid()`.

I think that will fix an issue when calling 
IOCTL_VM_SOCKETS_GET_LOCAL_CID and only vsock-loopback kernel module is 
loaded, so maybe we can do 2 patches:

1. fix IOCTL_VM_SOCKETS_GET_LOCAL_CID to check also `transport_local`
    Fixes: 0e12190578d0 ("vsock: add local transport support in the vsock core")

What would be the transport priority with transport_local thrown in? E.g.
if we have both local and g2h, ioctl should return VMADDR_CID_LOCAL or
transport_g2h->get_local_cid()?

2. move that code in vsock_get_local_cid() with proper locking and use 
it also in vsock_find_cid()

WDYT?

Yeah, sure about 1, I'll add it to the series. I'm just still not certain
how useful vsock_get_local_cid() would be for vsock_find_cid().

On Fri, 20 Jun 2025 at 16:23, Michal Luczaj [off-list ref] wrote:

On 6/20/25 15:20, Stefano Garzarella wrote:

quoted

On Fri, Jun 20, 2025 at 02:58:49PM +0200, Michal Luczaj wrote:

quoted

On 6/20/25 10:32, Stefano Garzarella wrote:

quoted

On Wed, Jun 18, 2025 at 02:34:00PM +0200, Michal Luczaj wrote:

quoted

Checking transport_{h2g,g2h} != NULL may race with vsock_core_unregister().
Make sure pointers remain valid.

KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
RIP: 0010:vsock_dev_do_ioctl.isra.0+0x58/0xf0
Call Trace:
__x64_sys_ioctl+0x12d/0x190
do_syscall_64+0x92/0x1c0
entry_SYSCALL_64_after_hwframe+0x4b/0x53

Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
Signed-off-by: Michal Luczaj <redacted>
---
net/vmw_vsock/af_vsock.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 2e7a3034e965db30b6ee295370d866e6d8b1c341..047d1bc773fab9c315a6ccd383a451fa11fb703e 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c

@@ -2541,6 +2541,8 @@ static long vsock_dev_do_ioctl(struct file *filp,

   switch (cmd) {
   case IOCTL_VM_SOCKETS_GET_LOCAL_CID:
+          mutex_lock(&vsock_register_mutex);
+
           /* To be compatible with the VMCI behavior, we prioritize the
            * guest CID instead of well-know host CID (VMADDR_CID_HOST).
            */

@@ -2549,6 +2551,8 @@ static long vsock_dev_do_ioctl(struct file *filp,
           else if (transport_h2g)
                   cid = transport_h2g->get_local_cid();

+          mutex_unlock(&vsock_register_mutex);

What about if we introduce a new `vsock_get_local_cid`:

u32 vsock_get_local_cid() {
    u32 cid = VMADDR_CID_ANY;

    mutex_lock(&vsock_register_mutex);
    /* To be compatible with the VMCI behavior, we prioritize the
     * guest CID instead of well-know host CID (VMADDR_CID_HOST).
     */
    if (transport_g2h)
            cid = transport_g2h->get_local_cid();
    else if (transport_h2g)
            cid = transport_h2g->get_local_cid();
    mutex_lock(&vsock_register_mutex);

    return cid;
}


And we use it here, and in the place fixed by next patch?

I think we can fix all in a single patch, the problem here is to call
transport_*->get_local_cid() without the lock IIUC.

Do you mean:

bool vsock_find_cid(unsigned int cid)
{
-       if (transport_g2h && cid == transport_g2h->get_local_cid())
+       if (transport_g2h && cid == vsock_get_local_cid())
               return true;

?

Nope, I meant:

  bool vsock_find_cid(unsigned int cid)
  {
-       if (transport_g2h && cid == transport_g2h->get_local_cid())
-               return true;
-
-       if (transport_h2g && cid == VMADDR_CID_HOST)
+       if (cid == vsock_get_local_cid())
                 return true;

         if (transport_local && cid == VMADDR_CID_LOCAL)

But it does change the behaviour, doesn't it? With this patch, (with g2h
loaded) if cid fails to match g2h->get_local_cid(), we don't fall back to
h2g case any more, i.e. no more comparing cid with VMADDR_CID_HOST.

It's friday... yep, you're right!

quoted

But now I'm thinking if we should also include `transport_local` in the
new `vsock_get_local_cid()`.

I think that will fix an issue when calling
IOCTL_VM_SOCKETS_GET_LOCAL_CID and only vsock-loopback kernel module is
loaded, so maybe we can do 2 patches:

1. fix IOCTL_VM_SOCKETS_GET_LOCAL_CID to check also `transport_local`
    Fixes: 0e12190578d0 ("vsock: add local transport support in the vsock core")

What would be the transport priority with transport_local thrown in? E.g.
if we have both local and g2h, ioctl should return VMADDR_CID_LOCAL or
transport_g2h->get_local_cid()?

Should return the G2H, LOCAL is more for debug/test, so I'd return it
only if anything else is loaded.

quoted

2. move that code in vsock_get_local_cid() with proper locking and use
it also in vsock_find_cid()

WDYT?

Yeah, sure about 1, I'll add it to the series. I'm just still not certain
how useful vsock_get_local_cid() would be for vsock_find_cid().

Feel free to drop 1 too, we can send it later if it's not really
related to this issue.
About the series, maybe it is better to have a single patch that fixes
the access to ->get_local_cid() with proper locking.
But I don't have a strong opinion on that. I see it like a single
problem to fix, but up to you.

Thanks,
Stefano

On 6/20/25 16:43, Stefano Garzarella wrote:

On Fri, 20 Jun 2025 at 16:23, Michal Luczaj [off-list ref] wrote:

quoted

On 6/20/25 15:20, Stefano Garzarella wrote:

quoted

On Fri, Jun 20, 2025 at 02:58:49PM +0200, Michal Luczaj wrote:

quoted

On 6/20/25 10:32, Stefano Garzarella wrote:

quoted

On Wed, Jun 18, 2025 at 02:34:00PM +0200, Michal Luczaj wrote:

quoted

Checking transport_{h2g,g2h} != NULL may race with vsock_core_unregister().
Make sure pointers remain valid.

KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
RIP: 0010:vsock_dev_do_ioctl.isra.0+0x58/0xf0
Call Trace:
__x64_sys_ioctl+0x12d/0x190
do_syscall_64+0x92/0x1c0
entry_SYSCALL_64_after_hwframe+0x4b/0x53

Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
Signed-off-by: Michal Luczaj <redacted>
---
net/vmw_vsock/af_vsock.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 2e7a3034e965db30b6ee295370d866e6d8b1c341..047d1bc773fab9c315a6ccd383a451fa11fb703e 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c

@@ -2541,6 +2541,8 @@ static long vsock_dev_do_ioctl(struct file *filp,

   switch (cmd) {
   case IOCTL_VM_SOCKETS_GET_LOCAL_CID:
+          mutex_lock(&vsock_register_mutex);
+
           /* To be compatible with the VMCI behavior, we prioritize the
            * guest CID instead of well-know host CID (VMADDR_CID_HOST).
            */

@@ -2549,6 +2551,8 @@ static long vsock_dev_do_ioctl(struct file *filp,
           else if (transport_h2g)
                   cid = transport_h2g->get_local_cid();

+          mutex_unlock(&vsock_register_mutex);

What about if we introduce a new `vsock_get_local_cid`:

u32 vsock_get_local_cid() {
    u32 cid = VMADDR_CID_ANY;

    mutex_lock(&vsock_register_mutex);
    /* To be compatible with the VMCI behavior, we prioritize the
     * guest CID instead of well-know host CID (VMADDR_CID_HOST).
     */
    if (transport_g2h)
            cid = transport_g2h->get_local_cid();
    else if (transport_h2g)
            cid = transport_h2g->get_local_cid();
    mutex_lock(&vsock_register_mutex);

    return cid;
}


And we use it here, and in the place fixed by next patch?

I think we can fix all in a single patch, the problem here is to call
transport_*->get_local_cid() without the lock IIUC.

Do you mean:

bool vsock_find_cid(unsigned int cid)
{
-       if (transport_g2h && cid == transport_g2h->get_local_cid())
+       if (transport_g2h && cid == vsock_get_local_cid())
               return true;

?

Nope, I meant:

  bool vsock_find_cid(unsigned int cid)
  {
-       if (transport_g2h && cid == transport_g2h->get_local_cid())
-               return true;
-
-       if (transport_h2g && cid == VMADDR_CID_HOST)
+       if (cid == vsock_get_local_cid())
                 return true;

         if (transport_local && cid == VMADDR_CID_LOCAL)

But it does change the behaviour, doesn't it? With this patch, (with g2h
loaded) if cid fails to match g2h->get_local_cid(), we don't fall back to
h2g case any more, i.e. no more comparing cid with VMADDR_CID_HOST.

It's friday... yep, you're right!

quoted

quoted

But now I'm thinking if we should also include `transport_local` in the
new `vsock_get_local_cid()`.

I think that will fix an issue when calling
IOCTL_VM_SOCKETS_GET_LOCAL_CID and only vsock-loopback kernel module is
loaded, so maybe we can do 2 patches:

1. fix IOCTL_VM_SOCKETS_GET_LOCAL_CID to check also `transport_local`
    Fixes: 0e12190578d0 ("vsock: add local transport support in the vsock core")

What would be the transport priority with transport_local thrown in? E.g.
if we have both local and g2h, ioctl should return VMADDR_CID_LOCAL or
transport_g2h->get_local_cid()?

Should return the G2H, LOCAL is more for debug/test, so I'd return it
only if anything else is loaded.

quoted

quoted

quoted

2. move that code in vsock_get_local_cid() with proper locking and use

it also in vsock_find_cid()

WDYT?

Yeah, sure about 1, I'll add it to the series. I'm just still not certain
how useful vsock_get_local_cid() would be for vsock_find_cid().

Feel free to drop 1 too, we can send it later if it's not really
related to this issue.

I've added it to the end of this series (and marked the series as RFC), for
ease of discussion.

About the series, maybe it is better to have a single patch that fixes
the access to ->get_local_cid() with proper locking.
But I don't have a strong opinion on that. I see it like a single
problem to fix, but up to you.

Yeah, I get your point. So I've tried something similar in v2:
https://lore.kernel.org/netdev/20250620-vsock-transports-toctou-v2-0-02ebd20b1d03@rbox.co/