Re: [PATCH mptcp-next v2 2/5] mptcp: pm: change path_manager sysctl atomically

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing module reference counting when setting the path manager sysctl leads to a Use-After-Free.
- [High] Missing module reference counting when setting the scheduler sysctl leads to a Use-After-Free.
--

--- Patch [2]: [PATCH mptcp-next v2 2/5] mptcp: pm: change path_manager sysctl atomically ---
commit c42ca00640178ef2df1ab9f51d1f65a9b722ec86
Author: Gang Yan <[email protected]>

mptcp: pm: change path_manager sysctl atomically

The per-netns path manager name is stored as an inline char[] buffer and
updated via strscpy() from the sysctl handler; a concurrent reader can
observe a half-written name. This patch replaces the string with a
pointer to the immutable struct mptcp_pm_ops.

Link: https://github.com/multipath-tcp/mptcp_net-next/issues/626

> Following the tcp_congestion_control() model (and the scheduler change in
> the previous patch), store a pointer to the immutable struct mptcp_pm_ops
> instead of the name string:

[Severity: High]
Since this patch follows the scheduler change in the previous patch, does
mptcp_set_scheduler() have the same module reference counting issue?

If mptcp_set_scheduler() assigns pernet->scheduler without acquiring a
module reference, and the BPF map is unloaded, could a new MPTCP socket
creation trigger mptcp_init_sock() which calls mptcp_get_scheduler(),
causing a Use-After-Free when it dereferences the freed sched pointer?

> diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
> index 479b31eb30079..c0481b09c1a17 100644
> --- a/net/mptcp/ctrl.c
> +++ b/net/mptcp/ctrl.c
[ ... ]
> @@ -85,9 +85,14 @@ int mptcp_get_pm_type(const struct net *net)
>  	return mptcp_get_pernet(net)->pm_type;
>  }
>  
> -const char *mptcp_get_path_manager(const struct net *net)
> +void mptcp_get_path_manager(const struct net *net, char *name)
>  {
> -	return mptcp_get_pernet(net)->path_manager;
> +	struct mptcp_pm_ops *pm_ops;
> +
> +	rcu_read_lock();
> +	pm_ops = rcu_dereference(mptcp_get_pernet(net)->path_manager);
> +	strscpy(name, pm_ops ? pm_ops->name : "kernel", MPTCP_PM_NAME_MAX);

[Severity: High]
If the module was unloaded and the pointer was not pinned, would this
dereference of pm_ops->name result in a Use-After-Free?

> +	rcu_read_unlock();
>  }
[ ... ]
> @@ -201,7 +206,7 @@ static int proc_blackhole_detect_timeout(const struct ctl_table *table,
[ ... ]
>  	rcu_read_lock();
>  	pm_ops = mptcp_pm_find(name);
>  	if (pm_ops)
> -		strscpy(path_manager, name, MPTCP_PM_NAME_MAX);
> +		xchg(&pernet->path_manager, pm_ops);

[Severity: High]
Does this assignment need to pin the module (e.g., via bpf_try_module_get())
before saving the pointer?

If a user with CAP_NET_ADMIN sets a BPF struct_ops map as the default path
manager, and then unloads the BPF program, the map is deleted and the
struct mptcp_pm_ops memory is freed.

Could this leave a dangling pointer in pernet->path_manager that causes the
Use-After-Free mentioned above when mptcp_get_path_manager() is subsequently
called?

>  	else
>  		ret = -ENOENT;
>  	rcu_read_unlock();

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2