From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qk2-f40.google.com (mail-qk2-f40.google.com [74.125.230.232]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 07E48231A3B for ; Sun, 27 Sep 2026 01:44:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=74.125.230.232 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790473463; cv=none; b=OyvLjLw2tj+E+uggstcEC5CHLMPr1g6BeaMPyDV09lVj03O7WwmF1Fmd13d9pA2kqnhA7NnBwAfKCth8T2UdYKsjxvQwlXo/xa0HYATButEOx64GfEO+zuvw/Jq8yQLEdPDB1lg1qNHzHlSPo41crz3DrF5RO4icNgvhFwRr4pY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790473463; c=relaxed/simple; bh=P5J9Sf16uPUvsZ+JfrZlq2hmQ3Ky4R4QrKaKwbraMmQ=; h=Mime-Version:Content-Type:Date:Message-Id:To:Cc:Subject:From: References:In-Reply-To; b=SNu+JZqLkxEk8HzFlAQaoYLuIk+du6bpVF0puDD1z89Aeuq4WgGuLXXQqvORzrVbdZr7YvrYqX4W7w0YMotZlorRChbmv1PTw4YGRjMdjjxiAG2zjzqytKTR4hXUFFiDB4Rugaf2wSFLpXz80AqFk6tng1g2WsAnn4EdfIiC1X8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=JoFggJVP; arc=none smtp.client-ip=74.125.230.232 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="JoFggJVP" Received: by mail-qk2-f40.google.com with SMTP id af79cd13be357-93c5ce9914dso79276685a.0 for ; Sat, 26 Sep 2026 18:44:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1790473461; x=1791078261; darn=vger.kernel.org; h=in-reply-to:references:from:subject:cc:to:message-id:date :content-type:content-transfer-encoding:mime-version:from:to:cc :subject:date:message-id:reply-to:content-type; bh=ZDvIRroBF25q1jkpXLqTWAFH4anbNjoJF32ra/1qtW8=; b=JoFggJVPQ8N2k4zi2zT7O79vhO51ZgBR69bMKH3FC+s6YmNZ7LjoDVBvP6HEaTM/J2 GsmZkTDqc4VdmA+6CkZqYbBi9xpmeHoarDTVNu4N0mqBBrfsv9FxD2AF0KaiYhou+M9m P/vJd1lKrsKpStd8NCCxgw7s5NwYmOSm+hhYcQaunYCVPoZbKZAD6kNnIMagXPzD/Hp0 hfa/O+bbVC+kUVJBI/3qC8uVFGnHLl/yEmFtquPc+5ofo0VmWBHj1I6lBs2a8C/NfJY0 QhNTP2gTJeoXzbTWRimMuG7eErBMuQek3EE8FqJayv/gPYje3bA0+aeSY2YEF3tRuuna MP2g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20260707; t=1790473461; x=1791078261; h=in-reply-to:references:from:subject:cc:to:message-id:date :content-type:content-transfer-encoding:mime-version:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=ZDvIRroBF25q1jkpXLqTWAFH4anbNjoJF32ra/1qtW8=; b=SeivU7e0tVBh1ZytQZYN4Ch1uz7Qnza5p3jp2NJt18YXfaT0EwoSMXdUGdtt8cdHbW Bknqw5ZjciYt3xQp9XGFb9CUGPmpvzu8gJcOnG1E52+T9hlXRUNeUNAzku66yGJhQ7bu IUhFFeffQrDBNNbEl3ozSTEULwPuxrW+hLIY4xNlzxFSCuHmpNGuSVvXPGzzXo+lrx5P VhzsCjhauYbBRYHQBe+OQVg+laS9BDmYUkKquj8n0dWoQZWKeIFFkYaEY7o8YBMmRKJ3 6jsS1BFQJDd0ztXTOOI8lzdcVsOuEWoIXLoZF+pZbaGbZ7m3DjWP+twiblEofXtBGYW1 Hk/w== X-Forwarded-Encrypted: i=1; AKwUvBxhWbQlrfvnGTUrCn2e4fjbQk9mK7t9Qw8Olb1Heo9PiUjuDElSosUFI5kn4opOhQkrKSN38800ybN8A+Q=@vger.kernel.org X-Gm-Message-State: AFuF++lpOeeBo0dQsmVrOjCk+tpi1LetzOw91PB/thMzaynb5cqPBWTT dZuBloQiZcpxAdL4DPEPZhJJVXTWEEr5ZtPg0t3qy58DsgV0lgVBX9La X-Gm-Gg: AYBFou3I607cAa+Pe7X8ViM+n/j70ysdyYQjnKAuwZNxBNROxH5/uGB2864jck1api7 XXKR4v0ml7bIA4RelYLVUPEvfBtW4/wYSE20a8upMQYzdLo7X4Ri3sl3cnAGa2GCfgv6Bik7+T6 cpP16Ikz5BGn4mcnMqAOQzaCsJD3ycrEP10qoWt3HAua2vlgCZ3Hvdn9gW4mkPHXaORNhc58Lgh pLJWdekDPXK6ijg0CBvjzkSidDiFuF4++wWxAd5qQGaV/EpR5M/ZT965hIxURBHI7ApFgjM44jg Pc9imq2pTUjaaSYZ2n2RFR1ezrmMwJadsvykpTpv3/xR2Ufm+zfMCz8nGQ48WyYpr7AI7s2TjoS biaBiQXFOFQCoHAg9SA47QxazGzTmrmx3fYpY6NzKw4sOm3WHG4rd+61IxIZP9g/Zj/qlJgMz84 VAgEeaMzGc8O8WMpyYjzXtsxsqLUaOkQgMvrfuXW7EbNsE2SXbWxA1kQzHC5rYk+eyhsdvnhsm3 klTjUCFoDFtPA== X-Received: by 2002:a05:620a:a515:b0:93c:5d68:107 with SMTP id af79cd13be357-93c5d683856mr415383485a.30.1790473460846; Sat, 26 Sep 2026 18:44:20 -0700 (PDT) Received: from localhost ([2600:4040:9399:4000:e553:72e5:7d37:c7ef]) by smtp.gmail.com with ESMTPSA id 6a1803df08f44-91430e0cb71sm50445796d6.31.2026.09.26.18.44.19 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 26 Sep 2026 18:44:20 -0700 (PDT) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Sat, 26 Sep 2026 21:44:19 -0400 Message-Id: To: , Cc: , , , , , , , , , , , , , , , , Subject: Re: [PATCH net-next 2/4] net: psp: require an established connection for association setup From: "Daniel Zahka" X-Mailer: aerc 0.21.0-threadmapfix References: <20260925-psp-defeat-v1-2-9f0b430107aa@gmail.com> <179047267826.2160803.3880931721750245623@kernel.org> In-Reply-To: <179047267826.2160803.3880931721750245623@kernel.org> On Sat Sep 26, 2026 at 9:31 PM EDT, netdev-bot+sashiko wrote: > Thank you for your contribution! Sashiko AI review found 3 potential > issue(s) to consider. > > Critical: 0 =C2=B7 High: 1 =C2=B7 Medium: 0 =C2=B7 Low: 2 > > If you believe none of the issues are legitimate, please reply to each > issue of Medium severity or higher and explain why the AI is wrong. > If any of them are legitimate and you plan to respin, please let patchwor= k > know by including "pw-bot: cr" as a separate line at the end of your repl= y > (one such reply per series is enough). > > - [High] The new `sk->sk_state !=3D TCP_ESTABLISHED` check in > psp_sock_assoc_set_tx() (net/psp/psp_sock.c:261-265) does not close=E2= =80=A6 > - [Low] The commit message says the tx-assoc check 'fixes a bug in commit > 6b46ca260e22 ("net: psp: add socket security association code")', but= =E2=80=A6 > - [Low] Baseline decision: false positive, kept only because > requires_validation=3Dtrue. > > -- > > net: psp: require an established connection for association setup > > With this commit, rx-assoc and tx-assoc return -ENOTCONN unless the socke= t > is in TCP_ESTABLISHED when checked under the socket lock. It also documen= ts > that the upgrade to PSP must happen on an established connection, and tha= t > disconnecting a socket with PSP assoc state is unsupported. The commit > message describes the tx-assoc check as a fix for an unsynchronized write > to an assoc shared with a timewait socket. > >> The check in the tx-assoc path fixes a bug in commit 6b46ca260e22 ("net: >> psp: add socket security association code") where an unsynchronized >> write can be performed an assoc shared with a timewait socket when the >> socket is in TCP_CLOSE after shutdown. This commit is not included in >> net, because its premise of preventing listen sockets from holding assoc >> state depends on the net-next commit 8cc3aef0cb19 ("tcp: Do not allow >> buggy transitions between ehash and lhash2.") > > [Severity: Low] > This is described as a fix for 6b46ca260e22, so should it carry a Fixes: > tag? Documentation/process/maintainer-netdev.rst says the Fixes: tag is > required for fixes regardless of the tree. > > The dependency on 8cc3aef0cb19 explains why this goes to net-next. That > dependency is about listen sockets, though. The tx-assoc case of TCP_CLOS= E > after shutdown does not seem to involve ehash/lhash2 transitions. > >> diff --git a/net/psp/psp_sock.c b/net/psp/psp_sock.c >> index a9cfeebe4ba1b..a6b1c42dd626f 100644 >> --- a/net/psp/psp_sock.c >> +++ b/net/psp/psp_sock.c > > [ ... ] > >> @@ -252,6 +258,12 @@ int psp_sock_assoc_set_tx(struct sock *sk, struct p= sp_dev *psd, >> =20 >> lock_sock(sk); >> =20 >> + if (sk->sk_state !=3D TCP_ESTABLISHED) { >> + NL_SET_ERR_MSG(extack, "Socket must be in established state"); >> + err =3D -ENOTCONN; >> + goto exit_unlock; >> + } >> + > > [Severity: High] > Does this check fully close the timewait sharing problem that the commit > message describes? A socket can return to TCP_ESTABLISHED after it has > already shared its psp_assoc with a timewait socket. > > psp_twsk_init() shares the assoc unconditionally, even when only the Rx > key is installed: > > net/psp/psp_sock.c:psp_twsk_init() { > struct psp_assoc *pas =3D psp_sk_assoc(sk); > > if (pas) > refcount_inc(&pas->refcnt); > rcu_assign_pointer(tw->psp_assoc, pas); > ... > } > > tcp_disconnect() does not touch sk->psp_assoc. The new psp.rst text says > the same thing: "Disconnect on a socket after rx-assoc will leave a socke= t > that can be reconnected, but with potentially stale PSP assoc state > present". > > rx-assoc and tx-assoc do not need admin permission, so this sequence look= s > reachable: > > connect(fd, peer1) > rx-assoc pas->tx.spi =3D=3D 0 > shutdown(fd, SHUT_WR) > peer FIN in FIN_WAIT2 > tcp_time_wait() > inet_twsk_alloc() > psp_twsk_init() tw->psp_assoc =3D pas > tcp_done() sk_state =3D TCP_CLOSE > connect(fd, AF_UNSPEC) > tcp_disconnect() sk->psp_assoc left in place > connect(fd, peer2) cleartext SYN/SYN-ACK allowed, !tx.spi > sk_state =3D TCP_ESTABLISHED > tx-assoc passes the new check > psp_sock_set_tx_key() > psp_assoc_set_tx() > memcpy(&pas->tx, key, sizeof(*key)); > pas->upgrade_seq =3D tcp_sk(sk)->rcv_nxt; > > Only the new connection's socket lock is held for these writes. The old > timewait socket is still hashed, and packets from peer1 go through: > > tcp_timewait_state_process() > psp_twsk_rx_policy_check() > __psp_sk_rx_policy_check() > > That path reads pas->tx.spi and pas->upgrade_seq and writes pas->peer_tx, > with only RCU protection: > > if (!pse) { > if (!pas->tx.spi || > (!pas->peer_tx && psp_is_allowed_nondata(skb, pas))) > return 0; > } > > Apart from the data race and possible torn reads, does the behavior of th= e > timewait socket also change? Once tx.spi is set for peer2, the timewait > socket would start dropping peer1's legitimate cleartext FIN retransmits > with SKB_DROP_REASON_PSP_INPUT. > > On the timewait reply path, psp_reply_set_decrypted() would also see > pas->tx.spi !=3D 0 and mark the ACK/RST replies to peer1 as decrypted. > psp_validate_xmit() then resolves tw->psp_assoc to the same pas, so those > old-flow replies would be PSP encapsulated with peer2's Tx key and SPI. > > The new documentation calls disconnect unsupported, but nothing prevents > it. Would the fix need to check whether the assoc is actually shared, or > reset or refuse assoc state across disconnect, instead of relying only on > a socket state check? Ok, I will need to rethink the approach a bit. pw-bot: cr