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/ (local)