Re: [PATCH mptcp-next v5 13/16] mptcp: add sk_is_msk() helper and use it in sockmap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Dereferencing a NULL function pointer (`sock->ops->peek_len`) when an MPTCP socket is attached to a sockmap or stream parser, causing a kernel panic.
- [High] MPTCP sockets are whitelisted for sockmap and stream parser operations without implementing the necessary BPF protocol hooks, leading to connection stalls, memory exhaustion, or dead code.
--

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

mptcp: add sk_is_msk() helper and use it in sockmap

This commit introduces the sk_is_msk() helper to identify MPTCP sockets.
It modifies sock_map.c to treat MPTCP sockets identically to TCP sockets
for stream parser linking, redirect checks, and state validation.

> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index 3e3e85e3bd54e..e9e0b676aa1ca 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -303,7 +303,7 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)
>  
>  	write_lock_bh(&sk->sk_callback_lock);
>  	if (stream_parser && stream_verdict && !psock->saved_data_ready) {
> -		if (sk_is_tcp(sk))
> +		if (sk_is_tcp(sk) || sk_is_msk(sk))
>  			ret = sk_psock_init_strp(sk, psock);

[Severity: High]
Does this code expose MPTCP sockets to a NULL pointer dereference?

By enabling stream parser initialization for MPTCP sockets here, we
override the socket operations and expose the stream parser paths.
However, MPTCP sockets do not implement the .peek_len operation
in mptcp_stream_ops.

When an MPTCP socket is attached to a sockmap, its ioctl is overridden
to tcp_bpf_ioctl(), which unconditionally calls it:

net/ipv4/tcp_bpf.c:tcp_bpf_ioctl() {
    ...
    *karg += sk->sk_socket->ops->peek_len(sk->sk_socket);
    ...
}

Similarly, the stream parser unconditionally calls it when parsing:

net/strparser/strparser.c:strp_peek_len() {
    ...
    return sock->ops->peek_len(sock);
}

Can an unprivileged user trigger a kernel panic by adding an MPTCP socket
to a sockmap and invoking ioctl(SIOCINQ) or receiving partial data?

[Severity: High]
Does this code also cause connection stalls and potential memory exhaustion
by whitelisting MPTCP for sockmap without implementing the required BPF
protocol hooks?

When an MPTCP socket is attached and processing SKBs, mptcp_read_skb()
unlinks the SKB for BPF verdict processing but entirely skips MPTCP's
internal consumption accounting (msk->bytes_consumed). This causes
mptcp_data_avail() to always return > 0, leading to mptcp_poll() busy-looping
with EPOLLIN at 100% CPU.

Additionally, when a stream parser is attached, sk_psock_init_strp()
skips overriding read_sock for MPTCP:

net/core/skmsg.c:sk_psock_init_strp() {
    ...
    if (sk_is_tcp(sk)) {
        psock->strp.cb.read_sock = tcp_bpf_strp_read_sock;
    ...
}

This forces a fallback to mptcp_read_sock(), which improperly uncharges the
SKB memory from the socket and ACKs the sender while the parser clones the
uncharged data into psock->ingress_msg. This bypasses socket memory limits,
allowing an attacker to stream infinite data and exhaust kernel memory.

Finally, MPTCP fails to provide a custom recvmsg parser for the BPF
configurations in mptcp_bpf_rebuild_protos(), falling back to mapping
mptcp_bpf_recvmsg directly to __tcp_bpf_recvmsg. This completely breaks
sequence tracking when SKB verdicts route data to the psock.

>  		else
>  			ret = -EOPNOTSUPP;

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