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@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
@@ -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
+ * until the last module_put() below or in vsock_deassign_transport().
+ */
+ mutex_unlock(&vsock_register_mutex);
if (sk->sk_type == SOCK_SEQPACKET) {
if (!new_transport->seqpacket_allow ||
@@ -528,6 +543,9 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
vsk->transport = new_transport;
return 0;
+err:
+ mutex_unlock(&vsock_register_mutex);
+ return ret;
}
EXPORT_SYMBOL_GPL(vsock_assign_transport);
--
2.49.0On 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@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?
Thanks,
Stefano
>
> /* 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
>+ * until the last module_put() below or in vsock_deassign_transport().
>+ */
>+ mutex_unlock(&vsock_register_mutex);
>
> if (sk->sk_type == SOCK_SEQPACKET) {
> if (!new_transport->seqpacket_allow ||
>@@ -528,6 +543,9 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
> vsk->transport = new_transport;
>
> return 0;
>+err:
>+ mutex_unlock(&vsock_register_mutex);
>+ return ret;
> }
> EXPORT_SYMBOL_GPL(vsock_assign_transport);
>
>
>--
>2.49.0
>
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@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?
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@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?
>
Nope, I reached the same conclusion!
Thanks,
Stefano
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@rbox.co>
>---
> net/vmw_vsock/af_vsock.c | 28 +++++++++++++++++++++++-----
> 1 file changed, 23 insertions(+), 5 deletions(-)
LGTM!
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>
>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
>@@ -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
>+ * until the last module_put() below or in vsock_deassign_transport().
>+ */
>+ mutex_unlock(&vsock_register_mutex);
>
> if (sk->sk_type == SOCK_SEQPACKET) {
> if (!new_transport->seqpacket_allow ||
>@@ -528,6 +543,9 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
> vsk->transport = new_transport;
>
> return 0;
>+err:
>+ mutex_unlock(&vsock_register_mutex);
>+ return ret;
> }
> EXPORT_SYMBOL_GPL(vsock_assign_transport);
>
>
>--
>2.49.0
>
© 2016 - 2025 Red Hat, Inc.