netdev - Re: [PATCH RFC net v2 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: <owafhdinyjdnol4zwpcdqsz26nfndawl53wnosdhhgmfz6t25n@2dualdqgpq3q>
Date: Fri, 27 Jun 2025 10:02:56 +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>, 
	Stefan Hajnoczi <stefanha@...hat.com>, virtualization@...ts.linux.dev, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC net v2 1/3] vsock: Fix transport_{g2h,h2g} TOCTOU

On Wed, Jun 25, 2025 at 11:23:30PM +0200, Michal Luczaj wrote:
>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@...hat.com>
>>> Signed-off-by: Michal Luczaj <mhal@...x.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?

Nope, you're right! I was still thinking about my old version where we 
had the switch inside...

BTW I'd like to change the name, `vsock_transport_local` prefix is 
confusing IMO, since it seems related only to the `transport_local`.

Another thing I'm worried about is that we'll then start using it on 
`vsk->transport` when this is only to be used on registered transports 
(i.e. `static ...`), though, I don't think there's a way to force type 
checking from the compiler (unless you wrap it in a struct). It's not a 
big issue, but taking the mutex is useless in that cases.

So, if we can't do much, I'd add a comment and make the function name 
more clear. e.g. vsock_registered_transport_cid() ? or something 
similar.

>
>>> +{
>>> +	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?

I usually prefer less changes as possibile, but in this case I see your 
point, so up to you ;-)

In your way we save `cid` initialization and an if, so it's fine.

Thanks,
Stefano

>
>if (transport_g2h)
>	cid = vsock_transport_local_cid(&transport_g2h);
>else if (transport_h2g)
>	cid = vsock_transport_local_cid(&transport_h2g);
>
>Thanks,
>Michal
>