netdev - Re: [PATCH net 1/3] vsock: Fix transport

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGxU2F65bh=jU6MVnhh=EzP19iayWATEezDFDd+c9o+K3Bf6YQ@mail.gmail.com>
Date: Fri, 20 Jun 2025 16:43:32 +0200
From: Stefano Garzarella <sgarzare@...hat.com>
To: Michal Luczaj <mhal@...x.co>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	virtualization@...ts.linux.dev, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net 1/3] vsock: Fix transport_{h2g,g2h} TOCTOU

On Fri, 20 Jun 2025 at 16:23, Michal Luczaj <mhal@...x.co> wrote:
>
> On 6/20/25 15:20, Stefano Garzarella wrote:
> > On Fri, Jun 20, 2025 at 02:58:49PM +0200, Michal Luczaj wrote:
> >> On 6/20/25 10:32, Stefano Garzarella wrote:
> >>> On Wed, Jun 18, 2025 at 02:34:00PM +0200, Michal Luczaj wrote:
> >>>> 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 <mhal@...x.co>
> >>>> ---
> >>>> 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!

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

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