On Sun, Jun 29, 2025 at 11:26:25PM +0200, Michal Luczaj wrote:

On 6/27/25 10:08, Stefano Garzarella wrote:

quoted

On Fri, Jun 20, 2025 at 09:52:44PM +0200, Michal Luczaj wrote:

quoted

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 <redacted>
---
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