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

[Michal Luczaj]

On 6/20/25 10:37, Stefano Garzarella wrote:
>> -if (!new_transport || !try_module_get(new_transport->module))
>> -return -ENODEV;
>> +if (!new_transport || !try_module_get(new_transport->module)) {
>> +ret = -ENODEV;
>> +goto unlock;
>> +}
>> +
> 
> I'd add a comment here to explain that we can release it since we
> successfully increased the `new_transport` refcnt.

Sure, will do.

>> +mutex_unlock(&vsock_register_mutex);
>>
>>  if (sk->sk_type == SOCK_SEQPACKET) {
>>  if (!new_transport->seqpacket_allow ||
>> @@ -528,6 +539,9 @@ int vsock_assign_transport(struct vsock_sock *vsk, 
>> struct vsock_sock *psk)
>>  vsk->transport = new_transport;
>>
>>  return 0;
>> +unlock:
> 
> I'd call it `err:` so it's clear is the error path.

Right, that makes sense.

Thanks!
Michal

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

[Stefano Garzarella]

On Wed, Jun 18, 2025 at 02:34:02PM +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: fbfff8056000
Oops: Oops:  [#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 
---
net/vmw_vsock/af_vsock.c | 24 +++-
1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 
337540efc237c8bc482a6730948fc773c00854f1..133d7c8d2231e5c2e5e6a697de3b104fe05d8020
 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 unlock;
}

if (vsk->transport) {
-   if (vsk->transport == new_transport)
-   return 0;
+   if (vsk->transport == new_transport) {
+   ret = 0;
+   goto unlock;
+   }

/* transport->release() must be called with sock lock acquired.
 * This path can only be taken during vsock_connect(), where we
@@ -508,8 +515,12 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct 
vsock_sock *psk)
/* We increase the module refcnt to prevent the transport unloading
 * while there are open sockets assigned to it.
 */
-   if (!new_transport || !try_module_get(new_transport->module))
-   return -ENODEV;
+   if (!new_transport || !try_module_get(new_transport->module)) {
+   ret = -ENODEV;
+   goto unlock;
+   }
+


I'd add a comment here to explain that we can release it since we
successfully increased the `new_transport` refcnt.


+   mutex_unlock(&vsock_register_mutex);

if (sk->sk_type == SOCK_SEQPACKET) {
if (!new_transport->seqpacket_allow ||
@@ -528,6 +539,9 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct 
vsock_sock *psk)
vsk->transport = new_transport;

return 0;
+unlock:


I'd call it `err:` so it's clear is the error path.

Thanks,
Stefano


+   mutex_unlock(&vsock_register_mutex);
+   return ret;
}
EXPORT_SYMBOL_GPL(vsock_assign_transport);


--
2.49.0