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 B950022618E for ; Thu, 19 Dec 2024 16:10:08 +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=1734624612; cv=none; b=d4jb/lyvclg7rFWV4nI1gT11zv8WjrJGImRkZNPMMXlxKVh1jPHkyYzuVpvwWmdt0VpRGBzT+HA2U0rvQyDEz2WFhhgLd2Qfw1Hqh/EPe4n3xXJm/r4LR3OZ9EGchw2LSKhPOIMFjNSoShApVWZRLURb+v40OW9HlqwUnkTC0Qs= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734624612; c=relaxed/simple; bh=w8uHLAOWytKhZ+2rc6CmLW4hjt/sBLkeIYoFGGtcGKc=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=B1J06lh5JWcv15WBVn+Ik5NB98vl9OYo+pRD2MEUGB+yscGRlMe0EJRTAFgG1nfZKRGFphTczMlBLCS/N4s22PVk0FZkDpBF3xS/tFz4uVo5g05eBVeTIoAda+0JQcA8TF+Y+G1213Zg8iFzd9CAhYOjDMNco83v9MTnJmMGzVw= 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=YHY1GRX2; 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="YHY1GRX2" 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 1tOJ63-008118-6S; Thu, 19 Dec 2024 17:09:55 +0100 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=QVsORav3DcVAhBYvUaAOANSrKkq0cKBzJoIMSAjqDVQ=; b=YHY1GRX20+/b0sq3p9K21iUEom dZjaWWOi64Qwp7hLn1BFKyXgIS1q1Grmj1rgvarCBURRp5Wxzcy6sCvOEcStIXUlNnBxUrUNnWfJV WRjwhqXbJXQJpfgWM0Nxxrq5/NHy5MpkxEQMup1jKXDd0/2fGOuofaUVyoT83iZPEyWAQmDx1FKP8 4dglqCWayHf9FNLWF7ktY84iym1Dvm3c+SNOJU5c4ciuyPZQOjWiF99VwyZfxmJurUhLTz/YKD7o/ EiBXkv42H0qxVq55KYhbBlFwZg3Up3MH5LVH2w3nTGuQwLaI/PErMhSQ7NKr3BsKSzA3npCOyimbg 1LejDZog==; Received: from [10.9.9.73] (helo=submission02.runbox) by mailtransmit03.runbox with esmtp (Exim 4.86_2) (envelope-from ) id 1tOJ62-0002R7-6c; Thu, 19 Dec 2024 17:09:54 +0100 Received: by submission02.runbox with esmtpsa [Authenticated ID (604044)] (TLS1.2:ECDHE_SECP256R1__RSA_PSS_RSAE_SHA256__AES_256_GCM:256) (Exim 4.93) id 1tOJ5r-00Ealw-Pp; Thu, 19 Dec 2024 17:09:43 +0100 Message-ID: <2906e706-bb0d-47c6-a4bb-9f3dc9ff7834@rbox.co> Date: Thu, 19 Dec 2024 17:09:42 +0100 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] vsock/virtio: Fix null-ptr-deref in vsock_stream_has_data To: Stefano Garzarella Cc: Hyunwoo Kim , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Simon Horman , Jason Wang , "Michael S. Tsirkin" , virtualization@lists.linux.dev, netdev@vger.kernel.org, qwerty@theori.io References: <5ca20d4c-1017-49c2-9516-f6f75fd331e9@rbox.co> <722e8d32-fe5c-4522-be2b-5967fdbb6b30@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 12/19/24 16:12, Stefano Garzarella wrote: > On Thu, 19 Dec 2024 at 16:05, Michal Luczaj wrote: >> >> On 12/19/24 15:48, Stefano Garzarella wrote: >>> On Thu, 19 Dec 2024 at 15:36, Michal Luczaj wrote: >>>> >>>> On 12/19/24 09:19, Stefano Garzarella wrote: >>>>> ... >>>>> I think the best thing though is to better understand how to handle >>>>> deassign, rather than checking everywhere that it's not null, also >>>>> because in some cases (like the one in virtio-vsock), it's also >>>>> important that the transport is the same. >>>> >>>> My vote would be to apply your virtio_transport_recv_pkt() patch *and* make >>>> it impossible-by-design to switch ->transport from non-NULL to NULL in >>>> vsock_assign_transport(). >>> >>> I don't know if that's enough, in this case the problem is that some >>> response packets are intended for a socket, where the transport has >>> changed. So whether it's null or assigned but different, it's still a >>> problem we have to handle. >>> >>> So making it impossible for the transport to be null, but allowing it >>> to be different (we can't prevent it from changing), doesn't solve the >>> problem for us, it only shifts it. >> >> Got it. I assumed this issue would be solved by `vsk->transport != >> &t->transport` in the critical place(s). >> >> (Note that BPF doesn't care if transport has changed; BPF just expects to >> have _a_ transport.) >> >>>> If I'm not mistaken, that would require rewriting vsock_assign_transport() >>>> so that a new transport is assigned only once fully initialized, otherwise >>>> keep the old one (still unhurt and functional) and return error. Because >>>> failing connect() should not change anything under the hood, right? >>>> >>> >>> Nope, connect should be able to change the transport. >>> >>> Because a user can do an initial connect() that requires a specific >>> transport, this one fails maybe because there's no peer with that cid. >>> Then the user can redo the connect() to a different cid that requires >>> a different transport. >> >> But the initial connect() failing does not change anything under the hood >> (transport should/could stay NULL). > > Nope, isn't null, it's assigned to a transport, because for example it > has to send a packet to connect to the remote CID and wait back for a > response that for example says the CID doesn't exist. Ahh, I think I get it. So the initial connect() passed vsock_assign_transport() successfully and then failed deeper in vsock_connect(), right? That's fine. Let the socket have a useless transport (a valid pointer nevertheless). Sure, upcoming connect() can assign a new (possibly useless just as well) transport, but there's no reason to allow ->transport becoming NULL. And a pre-connect socket (where ->transport==NULL) is not an issue, because BPF won't let it in any sockmap, so vsock_bpf_recvmsg() won't be reachable. Anywa, thanks for explaining, Michal PS. Or ignore the above and remove the socket from the sockmap at every reconnect? Possible unhash abuse: diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index 5cf8109f672a..8a65153ee186 100644 --- a/net/vmw_vsock/af_vsock.c +++ b/net/vmw_vsock/af_vsock.c @@ -483,6 +483,10 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk) if (vsk->transport == new_transport) return 0; + const struct proto *prot = READ_ONCE(sk->sk_prot); + if (prot->unhash) + prot->unhash(sk); + /* 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 diff --git a/net/vmw_vsock/vsock_bpf.c b/net/vmw_vsock/vsock_bpf.c index 4aa6e74ec295..80deb4d70aea 100644 --- a/net/vmw_vsock/vsock_bpf.c +++ b/net/vmw_vsock/vsock_bpf.c @@ -119,6 +119,7 @@ static void vsock_bpf_rebuild_protos(struct proto *prot, const struct proto *bas *prot = *base; prot->close = sock_map_close; prot->recvmsg = vsock_bpf_recvmsg; + prot->unhash = sock_map_unhash; prot->sock_is_readable = sk_msg_is_readable; } >> Then a successful re-connect assigns >> the transport (NULL -> non-NULL). And it's all good because all I wanted to >> avoid (because of BPF) was non-NULL -> NULL. Anyway, that's my possibly >> shallow understanding :)