Re: [PATCH RFC net v2 2/3] vsock: Fix transport_* TOCTOU

[Stefano Garzarella]

On Sun, Jun 29, 2025 at 11:26:25PM +0200, Michal Luczaj wrote:
> On 6/27/25 10:08, Stefano Garzarella wrote:
> > On Fri, Jun 20, 2025 at 09:52:44PM +0200, Michal Luczaj wrote:
> > > Transport assignment may race with module unload. Protect new_transport
> > > from becoming a stale pointer.
> > >
> > > This also takes care of an insecure call in vsock_use_local_transport();
> > > add a lockdep assert.
> > >
> > > BUG: unable to handle page fault for address: fffffbfff8056000
> > > Oops: Oops: 0000 [#1] SMP KASAN
> > > RIP: 0010:vsock_assign_transport+0x366/0x600
> > > Call Trace:
> > > vsock_connect+0x59c/0xc40
> > > __sys_connect+0xe8/0x100
> > > __x64_sys_connect+0x6e/0xc0
> > > 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@xxxxxxx>
> > > ---
> > > net/vmw_vsock/af_vsock.c | 28 +++++++++++++++++++++++-----
> > > 1 file changed, 23 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> > > index 63a920af5bfe6960306a3e5eeae0cbf30648985e..a1b1073a2c89f865fcdb58b38d8e7feffcf1544f 100644
> > > --- a/net/vmw_vsock/af_vsock.c
> > > +++ b/net/vmw_vsock/af_vsock.c
> > > @@ -407,6 +407,8 @@ EXPORT_SYMBOL_GPL(vsock_enqueue_accept);
> > >
> > > static bool vsock_use_local_transport(unsigned int remote_cid)
> > > {
> > > +	lockdep_assert_held(&vsock_register_mutex);
> > > +
> > > 	if (!transport_local)
> > > 		return false;
> > >
> > > @@ -464,6 +466,8 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
> > >
> > > 	remote_flags = vsk->remote_addr.svm_flags;
> > >
> > > +	mutex_lock(&vsock_register_mutex);
> > > +
> > > 	switch (sk->sk_type) {
> > > 	case SOCK_DGRAM:
> > > 		new_transport = transport_dgram;
> > > @@ -479,12 +483,15 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
> > > 			new_transport = transport_h2g;
> > > 		break;
> > > 	default:
> > > -		return -ESOCKTNOSUPPORT;
> > > +		ret = -ESOCKTNOSUPPORT;
> > > +		goto err;
> > > 	}
> > >
> > > 	if (vsk->transport) {
> > > -		if (vsk->transport == new_transport)
> > > -			return 0;
> > > +		if (vsk->transport == new_transport) {
> > > +			ret = 0;
> > > +			goto err;
> > > +		}
> >
> > 		/* transport->release() must be called with sock lock acquired.
> > 		 * This path can only be taken during vsock_connect(), where we
> > 		 * have already held the sock lock. In the other cases, this
> > 		 * function is called on a new socket which is not assigned to
> > 		 * any transport.
> > 		 */
> > 		vsk->transport->release(vsk);
> > 		vsock_deassign_transport(vsk);
> >
> > Thinking back to this patch, could there be a deadlock between call
> > vsock_deassign_transport(), which call module_put(), now with the
> > `vsock_register_mutex` held, and the call to vsock_core_unregister()
> > usually made by modules in the exit function?
>
> I think we're good. module_put() does not call the module cleanup function
> (kernel/module/main.c:delete_module() syscall does that), so
> vsock_core_unregister() won't happen in this path here. Have I missed
> anything else?

Nope, I reached the same conclusion!

Thanks,
Stefano