Re: [PATCH RFC net v2 1/3] vsock: Fix transport_{g2h,h2g} TOCTOU

[Michal Luczaj <mhal@rbox.co>]

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 <sgarzare@redhat.com>
>> Signed-off-by: Michal Luczaj <mhal@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