On 6/25/25 10:43, Stefano Garzarella wrote:
> On Fri, Jun 20, 2025 at 09:52:43PM +0200, Michal Luczaj wrote:
>> vsock_find_cid() and vsock_dev_do_ioctl() may race with module unload.
>> transport_{g2h,h2g} may become NULL after the NULL check.
>>
>> Introduce vsock_transport_local_cid() to 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
>>
>> 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")
>> Suggested-by: Stefano Garzarella <sgarz...@redhat.com>
>> Signed-off-by: Michal Luczaj <m...@rbox.co>
>> ---
>> net/vmw_vsock/af_vsock.c | 23 +++++++++++++++++------
>> 1 file changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index
>> 2e7a3034e965db30b6ee295370d866e6d8b1c341..63a920af5bfe6960306a3e5eeae0cbf30648985e
>> 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -531,9 +531,21 @@ int vsock_assign_transport(struct vsock_sock *vsk,
>> struct vsock_sock *psk)
>> }
>> EXPORT_SYMBOL_GPL(vsock_assign_transport);
>>
>> +static u32 vsock_transport_local_cid(const struct vsock_transport
>> **transport)
>
> Why we need double pointer?
Because of a possible race. If @transport is `struct vsock_transport*` and
we pass `transport_g2h`, the passed non-NULL pointer value may immediately
become stale (due to module unload). But if it's `vsock_transport**` and we
pass `&transport_g2h`, then we can take the mutex, check `*transport` for
NULL and safely go ahead.
Or are you saying this could be simplified?
>> +{
>> + u32 cid = VMADDR_CID_ANY;
>> +
>> + mutex_lock(&vsock_register_mutex);
>> + if (*transport)
>> + cid = (*transport)->get_local_cid();
>> + mutex_unlock(&vsock_register_mutex);
>> +
>> + return cid;
>> +}
>> +
>> bool vsock_find_cid(unsigned int cid)
>> {
>> - if (transport_g2h && cid == transport_g2h->get_local_cid())
>> + if (cid == vsock_transport_local_cid(&transport_g2h))
>> return true;
>>
>> if (transport_h2g && cid == VMADDR_CID_HOST)
>> @@ -2536,18 +2548,17 @@ static long vsock_dev_do_ioctl(struct file *filp,
>> unsigned int cmd, void __user *ptr)
>> {
>> u32 __user *p = ptr;
>> - u32 cid = VMADDR_CID_ANY;
>> int retval = 0;
>> + u32 cid;
>>
>> switch (cmd) {
>> case IOCTL_VM_SOCKETS_GET_LOCAL_CID:
>> /* 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();
>> + cid = vsock_transport_local_cid(&transport_g2h);
>> + if (cid == VMADDR_CID_ANY)
>> + cid = vsock_transport_local_cid(&transport_h2g);
>
> I still prefer the old `if ... else if ...`, what is the reason of this
> change? I may miss the point.
Ah, ok, I've just thought such cascade would be cleaner.
So is this what you prefer?
if (transport_g2h)
cid = vsock_transport_local_cid(&transport_g2h);
else if (transport_h2g)
cid = vsock_transport_local_cid(&transport_h2g);
Thanks,
Michal
