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 <m...@rbox.co>
>> ---
>> 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?
