From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailtransmit04.runbox.com (mailtransmit04.runbox.com [185.226.149.37]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 284C022B8BE for ; Fri, 20 Jun 2025 19:58:03 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.226.149.37 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1750449488; cv=none; b=mlBVfYGnePf8D/llulOLycq05TbRrZwLkrdFY7Xgknl2mRwRSna7uX3tO6+s74Xn6AMmjp3Gd35Cpoc5U9hXlt320f3wsOAkzOvnAaRpj36tNLp8Tm7Z2c4bA54o+WHDPCLhDar4hSXdGS3UJZep1jr/gZArbtWjz35C1XVhqrw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1750449488; c=relaxed/simple; bh=qD7QOF0Eu3kbnHtl32Gda5bek7meSJSxrmTH5Gei8i8=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=KZY+4DA9tUDkVCGZKlAPBaYvP/0fTpW0Fm+7GOyAX/9hfsQf189YHclFEy43HPvG6IsMIotcU844EFCTXum2t6Lo/Yx7SAFHcYVHHVvbVuI3mdG/qJaDyGwp0OldIaMFjgE/yB6fBV8ac4mFpGt8hEPG1Y8WUigwCQzdkw8swXs= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=rbox.co; spf=pass smtp.mailfrom=rbox.co; dkim=pass (2048-bit key) header.d=rbox.co header.i=@rbox.co header.b=v3EALXau; arc=none smtp.client-ip=185.226.149.37 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=rbox.co Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=rbox.co Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=rbox.co header.i=@rbox.co header.b="v3EALXau" Received: from mailtransmit03.runbox ([10.9.9.163] helo=aibo.runbox.com) by mailtransmit04.runbox.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.93) (envelope-from ) id 1uShs7-000fjV-GN; Fri, 20 Jun 2025 21:57:59 +0200 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=rbox.co; s=selector2; h=Content-Transfer-Encoding:Content-Type:In-Reply-To:From: References:Cc:To:Subject:MIME-Version:Date:Message-ID; bh=vckMvA6jWBSBfECI0pQWoIeT/beoNl3jiVpNm9w7QOU=; b=v3EALXauZCD/fezkh9p8YZEVyV 7pReDOY3G6YsYt0yWKrufLOu3MgvjZd4ElvASaHFJWXxkG+yR0DdQdajYx0WMv8/MmXzwcGZBTnYb njKy5NQpqFSICnPoQQt2Ah3bvyp4i5YapnANmBuhteTe7I+uJ4V0hu1qCLzaQwNCS/IFJf7MjwIwa sAx6BtbatjUfuErgB0zQyfirBQ+RtR+9w9q02otKoB1Ru0dKljoI7PRGzNm4lMfxGdTHCGV0sbFGp VYZKW+jNY7tm3a3YHRLRG0mO/Uv1S/DBMMQEngz9jHDGyRAXESNMlxeD9Ssql/zKyTHzmsqptaXOy 4lxOYanA==; Received: from [10.9.9.72] (helo=submission01.runbox) by mailtransmit03.runbox with esmtp (Exim 4.86_2) (envelope-from ) id 1uShs6-00044Z-Od; Fri, 20 Jun 2025 21:57:58 +0200 Received: by submission01.runbox with esmtpsa [Authenticated ID (604044)] (TLS1.2:ECDHE_SECP256R1__RSA_PSS_RSAE_SHA256__AES_256_GCM:256) (Exim 4.93) id 1uShrm-009FAR-5z; Fri, 20 Jun 2025 21:57:38 +0200 Message-ID: Date: Fri, 20 Jun 2025 21:57:36 +0200 Precedence: bulk X-Mailing-List: virtualization@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH net 1/3] vsock: Fix transport_{h2g,g2h} TOCTOU To: Stefano Garzarella Cc: "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Simon Horman , virtualization@lists.linux.dev, netdev@vger.kernel.org, linux-kernel@vger.kernel.org References: <20250618-vsock-transports-toctou-v1-0-dd2d2ede9052@rbox.co> <20250618-vsock-transports-toctou-v1-1-dd2d2ede9052@rbox.co> <4f0e2cc5-f3a0-4458-9954-438911e7d104@rbox.co> Content-Language: pl-PL, en-GB From: Michal Luczaj In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 6/20/25 16:43, Stefano Garzarella wrote: > On Fri, 20 Jun 2025 at 16:23, Michal Luczaj wrote: >> >> On 6/20/25 15:20, Stefano Garzarella wrote: >>> On Fri, Jun 20, 2025 at 02:58:49PM +0200, Michal Luczaj wrote: >>>> On 6/20/25 10:32, Stefano Garzarella wrote: >>>>> On Wed, Jun 18, 2025 at 02:34:00PM +0200, Michal Luczaj wrote: >>>>>> Checking transport_{h2g,g2h} != NULL may race with vsock_core_unregister(). >>>>>> Make sure pointers remain valid. >>>>>> >>>>>> KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f] >>>>>> RIP: 0010:vsock_dev_do_ioctl.isra.0+0x58/0xf0 >>>>>> Call Trace: >>>>>> __x64_sys_ioctl+0x12d/0x190 >>>>>> 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 | 4 ++++ >>>>>> 1 file changed, 4 insertions(+) >>>>>> >>>>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >>>>>> index 2e7a3034e965db30b6ee295370d866e6d8b1c341..047d1bc773fab9c315a6ccd383a451fa11fb703e 100644 >>>>>> --- a/net/vmw_vsock/af_vsock.c >>>>>> +++ b/net/vmw_vsock/af_vsock.c >>>>>> @@ -2541,6 +2541,8 @@ static long vsock_dev_do_ioctl(struct file *filp, >>>>>> >>>>>> switch (cmd) { >>>>>> case IOCTL_VM_SOCKETS_GET_LOCAL_CID: >>>>>> + mutex_lock(&vsock_register_mutex); >>>>>> + >>>>>> /* To be compatible with the VMCI behavior, we prioritize the >>>>>> * guest CID instead of well-know host CID (VMADDR_CID_HOST). >>>>>> */ >>>>>> @@ -2549,6 +2551,8 @@ static long vsock_dev_do_ioctl(struct file *filp, >>>>>> else if (transport_h2g) >>>>>> cid = transport_h2g->get_local_cid(); >>>>>> >>>>>> + mutex_unlock(&vsock_register_mutex); >>>>> >>>>> >>>>> What about if we introduce a new `vsock_get_local_cid`: >>>>> >>>>> u32 vsock_get_local_cid() { >>>>> u32 cid = VMADDR_CID_ANY; >>>>> >>>>> mutex_lock(&vsock_register_mutex); >>>>> /* To be compatible with the VMCI behavior, we prioritize the >>>>> * guest CID instead of well-know host CID (VMADDR_CID_HOST). >>>>> */ >>>>> if (transport_g2h) >>>>> cid = transport_g2h->get_local_cid(); >>>>> else if (transport_h2g) >>>>> cid = transport_h2g->get_local_cid(); >>>>> mutex_lock(&vsock_register_mutex); >>>>> >>>>> return cid; >>>>> } >>>>> >>>>> >>>>> And we use it here, and in the place fixed by next patch? >>>>> >>>>> I think we can fix all in a single patch, the problem here is to call >>>>> transport_*->get_local_cid() without the lock IIUC. >>>> >>>> Do you mean: >>>> >>>> bool vsock_find_cid(unsigned int cid) >>>> { >>>> - if (transport_g2h && cid == transport_g2h->get_local_cid()) >>>> + if (transport_g2h && cid == vsock_get_local_cid()) >>>> return true; >>>> >>>> ? >>> >>> Nope, I meant: >>> >>> bool vsock_find_cid(unsigned int cid) >>> { >>> - if (transport_g2h && cid == transport_g2h->get_local_cid()) >>> - return true; >>> - >>> - if (transport_h2g && cid == VMADDR_CID_HOST) >>> + if (cid == vsock_get_local_cid()) >>> return true; >>> >>> if (transport_local && cid == VMADDR_CID_LOCAL) >> >> But it does change the behaviour, doesn't it? With this patch, (with g2h >> loaded) if cid fails to match g2h->get_local_cid(), we don't fall back to >> h2g case any more, i.e. no more comparing cid with VMADDR_CID_HOST. > > It's friday... yep, you're right! > >> >>> But now I'm thinking if we should also include `transport_local` in the >>> new `vsock_get_local_cid()`. >>> >>> I think that will fix an issue when calling >>> IOCTL_VM_SOCKETS_GET_LOCAL_CID and only vsock-loopback kernel module is >>> loaded, so maybe we can do 2 patches: >>> >>> 1. fix IOCTL_VM_SOCKETS_GET_LOCAL_CID to check also `transport_local` >>> Fixes: 0e12190578d0 ("vsock: add local transport support in the vsock core") >> >> What would be the transport priority with transport_local thrown in? E.g. >> if we have both local and g2h, ioctl should return VMADDR_CID_LOCAL or >> transport_g2h->get_local_cid()? > > Should return the G2H, LOCAL is more for debug/test, so I'd return it > only if anything else is loaded. >>>> 2. move that code in vsock_get_local_cid() with proper locking and use >>> it also in vsock_find_cid() >>> >>> WDYT? >> >> Yeah, sure about 1, I'll add it to the series. I'm just still not certain >> how useful vsock_get_local_cid() would be for vsock_find_cid(). >> > > Feel free to drop 1 too, we can send it later if it's not really > related to this issue. I've added it to the end of this series (and marked the series as RFC), for ease of discussion. > About the series, maybe it is better to have a single patch that fixes > the access to ->get_local_cid() with proper locking. > But I don't have a strong opinion on that. I see it like a single > problem to fix, but up to you. Yeah, I get your point. So I've tried something similar in v2: https://lore.kernel.org/netdev/20250620-vsock-transports-toctou-v2-0-02ebd20b1d03@rbox.co/