[PATCH mptcp-net] mptcp: pm: fix potential NULL deref in mptcp_pm_ops_init

[PATCH mptcp-net] mptcp: pm: fix potential NULL deref in mptcp_pm_ops_init

[Gang Yan]

From: Gang Yan <yangang@kylinos.cn>

mptcp_pm_find will return NULL when the PM is not registered. In that
case, mptcp_pm_ops_init will make it fallback to kernel_pm, and the
pr_warn_once will dereferences pm_ops->name.

This patch uses 'UNKNOWN' in pr_war_once to avoid this issue when the
pm_ops is NULL.

Signed-off-by: Gang Yan <yangang@kylinos.cn>
---

Notes:
    Hi Matt, Geliang
    
    This patch fixes a potential issue that might be encountered by the
    future eBPF path manager. However, as far as I know, this issue should
    not be triggered in the current codebase.
    
    That said, it could potentially be triggered in the future when the BPF
    Path Manager is in use. For example, in our current self-test
    environment, within a network namespace, we use a BPF-type path manager
    (specified via sysctl). If the path manager is unregistered and a new
    msk is created in that namespace, the code path would hit this and
    trigger a NULL pointer dereference.
    
    Therefore, I would suggest adding a simple check/handling here to make
    it more robust. WDYT?
    
    Thanks
    Gang

 net/mptcp/pm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 9dc7b41fb562..0ba6dd25ba49 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -1142,7 +1142,7 @@ static void mptcp_pm_ops_init(struct mptcp_sock *msk,
 {
 	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 ? pm_ops->name : "UNKNOWN");
 		pm_ops = &mptcp_pm_kernel;
 	}
 
-- 
2.43.0

Re: [PATCH mptcp-net] mptcp: pm: fix potential NULL deref in mptcp_pm_ops_init

[MPTCP CI]

Hi Gang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join): Success! ✅
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/28430160954

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/b0b9e97629b2
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1118737


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH mptcp-net] mptcp: pm: fix potential NULL deref in mptcp_pm_ops_init

[Matthieu Baerts]

Hi Gang,

Thank you for looking at that.

> mptcp_pm_find will return NULL when the PM is not registered. In that
> case, mptcp_pm_ops_init will make it fallback to kernel_pm, and the
> pr_warn_once will dereferences pm_ops->name.
>
> This patch uses 'UNKNOWN' in pr_war_once to avoid this issue when the
> pm_ops is NULL.

Patches for 'net' (c.f. 'mptcp-net' prefix) should have a Fixes tag. But
this patch modifies code that is only in our tree (introduced by "mptcp:
pm: init and release mptcp_pm_ops"). Can you then send a Squash-to patch
instead, please?

> Signed-off-by: Gang Yan <yangang@kylinos.cn>

About your note, I think it is fine to handle the case when pm_ops is
NULL here, as this code will be used when the BPF PM will be fully
introduced.

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 9dc7b41fb562..0ba6dd25ba49 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -1142,7 +1142,7 @@ static void mptcp_pm_ops_init(struct mptcp_sock *msk,
>  {
>  	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 ? pm_ops->name : "UNKNOWN");

Instead of this, I think it would be better to pass the PM name to this
function, and call mptcp_pm_find() from here. By doing that, you can
print the original string passed in argument, and no need to have this
fallback to UNKNOWN.

While at it, a fix in mptcp_pm_ops_release() is also needed, to move the
pr_debug() before calling bpf_module_put().

Cheers,
Matt

-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>