Re: [PATCH net v7] kcm: reject TLS on attached TCP sockets

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

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 patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The Fixes: tag covers only one of the two defects the commit
  message says it fixes.

--

kcm: reject TLS on attached TCP sockets

This patch makes kcm_attach() reject TCP sockets that already have a ULP
installed. It also makes tls_init() reject sockets attached to KCM, using
a new kcm_attached flag in tcp_sock. The kcm_attach() type check now uses
sk_is_tcp(), so raw sockets created with IPPROTO_TCP are rejected.

Link: https://lore.kernel.org/netdev/3108e934-ec00-458b-8720-450554b35b55@linux.dev/

> KCM calls read_sock() from its receive callback in atomic context. With
> kTLS RX, this reaches tls_sw_read_sock(), which can sleep. KCM and TLS
> also replace socket callbacks, so detaching KCM can discard TLS handlers.

[ ... ]

> Fixes: 662fbcec32f4 ("net/tls: implement ->read_sock()")
> Cc: <stable@vger.kernel.org> # see backport requirement above

[Severity: Medium]

Does the Fixes: tag cover both of the problems described above?

662fbcec32f4 added tls_sw_read_sock() as the read_sock op, which explains
the sleep in atomic context. The callback replacement problem looks older.
In the parent of 662fbcec32f4, neither kcm_attach() nor tls_init() checked
for the other, so the two could already be stacked.

If KCM is attached first and TLS_RX is enabled afterwards,
tls_sw_strparser_arm() saves psock_data_ready and installs tls_data_ready:

net/tls/tls_sw.c:tls_sw_strparser_arm() {
    ...
	rx_ctx->saved_data_ready = sk->sk_data_ready;
	sk->sk_data_ready = tls_data_ready;
    ...
}

On SIOCKCMUNATTACH, kcm_unattach() then writes the pre-KCM callback back
over tls_data_ready:

net/kcm/kcmsock.c:kcm_unattach() {
    ...
	WRITE_ONCE(csk->sk_data_ready, psock->save_data_ready);
    ...
}

After that, TLS RX is no longer notified.

Before 662fbcec32f4 the TLS_SW proto_ops also inherited read_sock from
inet_stream_ops. So in either order, strp_read_sock() would call
tcp_read_sock(). KCM's strparser would then take raw TLS ciphertext from
the same receive queue that TLS parses.

This seems to go back to c46234ebb4d1 ("tls: RX path for ktls"). Stable
selection uses the Fixes: commit. Would LTS trees without 662fbcec32f4
(6.1, and 5.15/5.10 while they are still maintained) skip this patch? On
those trees an unprivileged user could still stack KCM and kTLS RX.

Could the Fixes: tag point at c46234ebb4d1, or could the Cc: stable note
say whether pre-6.6 trees are affected? Right now the note only mentions
the 8cc3aef0cb198 dependency.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261112232444.2865382-1-rjethwani%40purestorage.com