On 7/3/25 10:20, Stefano Garzarella wrote:
> On Wed, Jul 02, 2025 at 03:38: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")
>> Reviewed-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>
>> 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 39473b9e0829f240045262aef00cbae82a425dcc..9b2af5c63f7c2ae575c160415bd77208a3980835 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
>> @@ -508,8 +515,16 @@ 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 err;
>> + }
>> +
>> + /* It's safe to release the mutex after a successful try_module_get().
>> + * Whichever transport `new_transport` points at, it won't go await
>
> Little typo, s/await/away
>
> Up to you to resend or not. My R-b stay for both cases.
Arrgh, thanks. I'll fix it.
pw-bot: changes-requested
