Re: [PATCH net v2] mptcp: protect listener backlog reads in diagnostics

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mptcp: UAF in mptcp_diag_get_info() as __mptcp_close_ssk() puts ssk first
--

commit 54c0f83180af04b33f5796a4675ffc1cc6ddce9a
Author: Cen Zhang <zzzccc427@gmail.com>

mptcp: protect listener backlog reads in diagnostics

This commit wraps the msk->first lookup and the subsequent listener backlog
reads in mptcp_diag_get_info() with rcu_read_lock() and rcu_read_unlock() to
prevent a use-after-free when a listening MPTCP socket is closed concurrently
with a token diagnostic dump.

> diff --git a/net/mptcp/mptcp_diag.c b/net/mptcp/mptcp_diag.c
> index 136c2d05c0eeb..28a42eee66512 100644
> --- a/net/mptcp/mptcp_diag.c
> +++ b/net/mptcp/mptcp_diag.c
> @@ -200,8 +200,10 @@ static void mptcp_diag_get_info(struct sock *sk, struct inet_diag_msg *r,
>  	r->idiag_wqueue = sk_wmem_alloc_get(sk);
>  
>  	if (inet_sk_state_load(sk) == TCP_LISTEN) {
> -		struct sock *lsk = READ_ONCE(msk->first);
> +		struct sock *lsk;
>  
> +		rcu_read_lock();
> +		lsk = READ_ONCE(msk->first);
>  		if (lsk) {
>  			/* override with settings from tcp listener,
>  			 * so Send-Q will show accept queue.
> @@ -209,6 +211,7 @@ static void mptcp_diag_get_info(struct sock *sk, struct inet_diag_msg *r,
>  			r->idiag_rqueue = READ_ONCE(lsk->sk_ack_backlog);
>  			r->idiag_wqueue = READ_ONCE(lsk->sk_max_ack_backlog);
>  		}
> +		rcu_read_unlock();

[Severity: High]
Can this still read from a freed lsk if mptcp_diag_get_info() in
net/mptcp/mptcp_diag.c races with __mptcp_close_ssk() in
net/mptcp/protocol.c?

In net/mptcp/protocol.c:__mptcp_close_ssk(), the final reference on ssk is
dropped before msk->first is cleared:

out_release:
	__mptcp_subflow_error_report(sk, ssk);
	release_sock(ssk);

	sock_put(ssk);

	if (ssk == msk->first)
		WRITE_ONCE(msk->first, NULL);

After release_sock(ssk), the closing task runs in preemptible process context
holding only the msk socket lock. Calling sock_put(ssk) initiates RCU
reclamation via sk_free() -> __sk_free() -> sk_destruct() -> call_rcu() while
msk->first still points to ssk.

In net/mptcp/mptcp_diag.c:mptcp_diag_get_info(), the state check happens
before entering the RCU read-side critical section:

	if (inet_sk_state_load(sk) == TCP_LISTEN) {
		struct sock *lsk;

		rcu_read_lock();
		lsk = READ_ONCE(msk->first);

If mptcp_diag_get_info() observes TCP_LISTEN and is preempted before calling
rcu_read_lock(), and concurrently close() -> __mptcp_close() ->
mptcp_destroy() -> mptcp_destroy_common() -> __mptcp_close_ssk() calls
sock_put(ssk) and is preempted before WRITE_ONCE(msk->first, NULL), the RCU
grace period for ssk can complete while msk->first remains non-NULL. When
mptcp_diag_get_info() resumes and enters rcu_read_lock(), wouldn't
READ_ONCE(msk->first) still load the freed ssk pointer and dereference
lsk->sk_ack_backlog and lsk->sk_max_ack_backlog?

Should __mptcp_close_ssk() clear msk->first with WRITE_ONCE(msk->first, NULL)
before calling sock_put(ssk), matching the ordering in mptcp_sk_clone_init()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/pm-mptcp-objects-candidate-0066-v5-4d1b83d62fcde2d57e8d@gmail.com?part=1