Re: [PATCH mptcp-next v5 07/16] mptcp: add sock_map_update BPF helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The verifier incorrectly allows bpf_mptcp_sock_map_update to be used with BPF_MAP_TYPE_SOCKHASH and other arbitrary map types, but its implementation treats the map strictly as an array-based BPF_MAP_TYPE_SOCKMAP. This causes type confusion and severe out-of-bounds memory corruption.
- [High] bpf_mptcp_sock_map_update modifies the parent MPTCP socket (msk) state without holding its socket lock, leading to data races with userspace and MPTCP stack operations.
- [High] bpf_mptcp_sock_map_update bypasses necessary socket state and operation context validation (sock_map_op_okay), allowing transient state MPTCP sockets or ANY standard TCP socket to be unsafely inserted into a sockmap.
--

commit d25c16225b28c5b937139f3df217e21f3cd9bb7b
Author: Geliang Tang <geliang@kernel.org>

    mptcp: add sock_map_update BPF helper
    
    Add a BPF helper bpf_mptcp_sock_map_update() that allows sockops programs
    running on MPTCP subflows to insert the parent MPTCP socket into a sockmap
    or sockhash. When called from a sockops callback on a subflow, it resolves
    the parent MPTCP socket via bpf_mptcp_sock_from_subflow() and validates
    that the subflow is the first subflow (msk->first) before inserting the
    MPTCP socket into the map via sock_map_update_common().

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 72a3f5998dd27..c58c2ba87d264 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -9075,6 +9076,7 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env,
>  		    func_id != BPF_FUNC_msg_redirect_hash &&
>  		    func_id != BPF_FUNC_sk_select_reuseport &&
>  		    func_id != BPF_FUNC_map_lookup_elem &&
> +		    func_id != BPF_FUNC_mptcp_sock_map_update &&
>  		    !may_update_sockmap(env, func_id))
>  			goto error;
>  		break;

[Severity: Critical]
Does allowing BPF_FUNC_mptcp_sock_map_update for BPF_MAP_TYPE_SOCKHASH maps
cause type confusion?

Because mptcp_sock_map_update() calls sock_map_update_common() which casts
the map to struct bpf_stab, passing a SOCKHASH (backed by struct bpf_shtab)
will write the socket pointer into what it thinks is the stab->sks array.
Couldn't this overlap with hash bucket lists or spinlocks and corrupt memory?

Additionally, because BPF_FUNC_mptcp_sock_map_update is omitted from the
subsequent switch (func_id) statement in check_map_func_compatibility(),
could this helper be incorrectly allowed for other unrestricted map types
like ARRAY or HASH?

> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index 662b80ce2cf63..7ecafbf51fa05 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c
[ ... ]
> +BPF_CALL_4(mptcp_sock_map_update, struct bpf_sock_ops_kern *, sops,
> +	   struct bpf_map *, map, void *, key, u64, flags)
> +{
> +	struct sock *sk = sops->sk;
> +	struct mptcp_sock *msk;
> +
> +	WARN_ON_ONCE(!rcu_read_lock_held());
> +
> +	msk = bpf_mptcp_sock_from_subflow(sk);
> +	if (msk) {

[Severity: High]
Does this bypass the socket state and operation validation checks normally
performed before inserting a socket into a sockmap?

If a BPF program calls this helper from an early hook (e.g.,
BPF_SOCK_OPS_TCP_CONNECT_CB) to insert a TCP_SYN_SENT socket, it appears
unprepared or closing sockets could be inserted, violating subsystem
invariants.

Furthermore, if the socket is a standard TCP socket rather than an MPTCP
subflow, bpf_mptcp_sock_from_subflow() returns NULL. Would this cause the
helper to silently fall through and insert the standard TCP socket into the
map, completely bypassing checks like sock_map_op_okay() and
sock_map_sk_is_suitable()?

> +		if (sk != READ_ONCE(msk->first))
> +			return -EINVAL;
> +
> +		sk = (struct sock *)msk;

[Severity: High]
Does modifying the parent MPTCP socket (msk) state without holding its socket
lock lead to data races with userspace and MPTCP stack operations?

When called from a sockops hook running on a subflow socket, the network stack
only holds the subflow socket lock, not the parent MPTCP socket lock.
Because sock_map_update_common() modifies the socket's protocol operations
(sk->sk_prot) and sets sk_user_data, could modifying sk_prot concurrently
while another CPU executes userspace system calls (e.g., recvmsg or sendmsg)
on the parent socket cause use-after-free crashes or data corruption?

> +	}
> +
> +	return sock_map_update_common(map, *(u32 *)key, sk, flags);

[Severity: Critical]
Does unconditionally calling the array-based sockmap update logic here cause
type confusion when a hash map is supplied?

Also, doesn't this truncate variable-length hash keys to u32?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789294365.git.tanggeliang@kylinos.cn?part=7