Re: [PATCH mptcp-next v7 6/6] Squash to previous one

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mptcp: uaf and uninitialized access in pm_ops during disconnect
--

commit 89b80a47ef923bdc04a9069fdcf06de447841c5b
Author: Gang Yan <yangang@kylinos.cn>

Squash to previous one

This patch applies RCU discipline to msk->pm.ops, moving its initialization,
publication, and release to better align with RCU patterns. It updates readers
to use rcu_dereference() and writers to use rcu_assign_pointer() under the
PM lock.

Link: https://sashiko.dev/#/patchset/20260819125629.49823-1-gang.yan@linux.dev?part=5

>     mptcp_destroy(), the final close path: mptcp_token_destroy() has
>     already removed the socket from the token hash by then, so no new
>     reader can find the msk anymore.
>     
>     Note that rcu_assign_pointer() before pm_ops->init(msk) does not
>     publish a partially initialised object: the ops are registered
>     immutable, init() only prepares the per-socket state, and the msk is
>     not reachable by readers until its token is registered.

[Severity: High]
Is this assumption accurate? A concurrent SoftIRQ can receive an MP_JOIN
request, look up the socket via mptcp_token_get_sock(), increment its refcnt,
and drop the RCU read lock. Concurrently, the socket could be disconnected,
calling mptcp_disconnect() -> mptcp_pm_data_reset() -> mptcp_pm_ops_init().

The SoftIRQ thread could then re-acquire the RCU read lock and call
mptcp_pm_get_local_id(), accessing the actively tearing down or uninitialized
per-socket state.

[ ... ]
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 64244a1a01bca..66fb7c3d9c827 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
[ ... ]
> @@ -1158,23 +1166,39 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
>  static void mptcp_pm_ops_init(struct mptcp_sock *msk,
>  			      struct mptcp_pm_ops *pm_ops)
>  {
> -	if (!pm_ops || !bpf_try_module_get(pm_ops, pm_ops->owner)) {
> -		pr_warn_once("pm %s fails, fallback to default pm", pm_ops->name);
> -		pm_ops = &mptcp_pm_kernel;
> +	struct mptcp_pm_ops *old;
> +
> +	spin_lock_bh(&msk->pm.lock);
> +	old = rcu_dereference_protected(msk->pm.ops,
> +					lockdep_is_held(&msk->pm.lock));
> +	if (old != pm_ops)
> +		rcu_assign_pointer(msk->pm.ops, pm_ops);

[Severity: High]
Does this publish the new ops to readers before its state is initialized via
init(msk) below? If a reader finds the socket, it could access the new ops
before its per-socket state is ready.

> +	spin_unlock_bh(&msk->pm.lock);
> +
> +	if (old) {
> +		synchronize_rcu();
> +		if (old->release)
> +			old->release(msk);

[Severity: High]
If old == pm_ops, we skip the rcu_assign_pointer() above, meaning the ops
are still actively published in msk->pm.ops. Does calling old->release(msk)
here violate RCU removal rules by releasing the ops state while it is still
reachable by readers?

> +		bpf_module_put(old, old->owner);
>  	}
>  
> -	msk->pm.ops = pm_ops;
> -	if (msk->pm.ops->init)
> -		msk->pm.ops->init(msk);
> +	if (pm_ops->init)
> +		pm_ops->init(msk);
>  
>  	pr_debug("pm %s initialized\n", pm_ops->name);
>  }
[ ... ]
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 14aa82647c3d1..4953cfa329e81 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -94,14 +94,17 @@ static struct mptcp_sock *subflow_token_join_request(struct request_sock *req)
>  		return NULL;
>  	}
>  
> +	rcu_read_lock();
>  	local_id = mptcp_pm_get_local_id(msk, (struct sock_common *)req);

[Severity: High]
This is where a concurrent reader accesses the PM ops using a cached msk
reference. Does this disprove the assumption in the commit message that
the socket is unreachable, since the reader already holds a reference to msk?

>  	if (local_id < 0) {
>  		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPJOINNOIDFOUND);
> +		rcu_read_unlock();
>  		sock_put((struct sock *)msk);
>  		return NULL;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790147728.git.yangang@kylinos.cn?part=6