Re: [PATCH mptcp-next v5 10/16] bpf: export and generalize tcp_bpf_ioctl

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Calling `sk->sk_prot->ioctl(sk, cmd, karg)` from within `tcp_bpf_ioctl` unconditionally causes an infinite recursive loop, leading to kernel stack exhaustion and a panic (DoS).
- [Critical] Dereferencing `sk->sk_socket->ops->peek_len` unconditionally crashes on MPTCP sockets because their protocol operations do not implement the `.peek_len` callback.
--

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

bpf: export and generalize tcp_bpf_ioctl

This commit exports tcp_bpf_ioctl() and generalizes it to use
protocol-specific operations instead of hard-coded TCP functions. It also
wires this generalized ioctl handler into the MPTCP BPF protocol structures
so that SIOCINQ returns correct results accounting for data in the psock
ingress queue.

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index e006131696e96..8aed3c309b0dc 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
[ ... ]
> @@ -331,15 +331,15 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>  	return copied;
>  }
>  
> -static int tcp_bpf_ioctl(struct sock *sk, int cmd, int *karg)
> +int tcp_bpf_ioctl(struct sock *sk, int cmd, int *karg)
>  {
>  	struct sk_psock *psock;
>  	bool slow;
>  
>  	if (cmd != SIOCINQ)
> -		return tcp_ioctl(sk, cmd, karg);
> +		return sk->sk_prot->ioctl(sk, cmd, karg);

[Severity: Critical]
Does this cause an infinite recursive loop regression?

When a socket is attached to a BPF sockmap, its protocol operations
structure is replaced by a BPF-specific protocol structure. Since this patch
assigns .ioctl = tcp_bpf_ioctl in those structures, sk->sk_prot->ioctl will
resolve right back to tcp_bpf_ioctl itself, exhausting the kernel stack and
leading to a system panic.

>  
> -	/* works similar as tcp_ioctl */
> +	/* works similar as sk_prot->ioctl */
>  	if (sk->sk_state == TCP_LISTEN)
>  		return -EINVAL;
>  
> @@ -347,7 +347,7 @@ static int tcp_bpf_ioctl(struct sock *sk, int cmd, int *karg)
>  	psock = sk_psock_get(sk);
>  	if (unlikely(!psock)) {
>  		unlock_sock_fast(sk, slow);
> -		return tcp_ioctl(sk, cmd, karg);
> +		return sk->sk_prot->ioctl(sk, cmd, karg);

[Severity: Critical]
Will this fallback path also trigger the same infinite recursion regression
as above?

>  	}
>  	*karg = sk_psock_get_msg_len_nolock(psock);
>  	/* Without a verdict program, ingress data is never diverted to
> @@ -357,7 +357,7 @@ static int tcp_bpf_ioctl(struct sock *sk, int cmd, int *karg)
>  	 */
>  	if (!READ_ONCE(psock->progs.stream_verdict) &&
>  	    !READ_ONCE(psock->progs.skb_verdict))
> -		*karg += tcp_inq(sk);
> +		*karg += sk->sk_socket->ops->peek_len(sk->sk_socket);

[Severity: Critical]
Will this cause a NULL pointer dereference regression for MPTCP sockets?

While TCP's socket operations define a .peek_len callback, MPTCP's socket
operations do not implement it, leaving the function pointer initialized to
NULL. Dereferencing it unconditionally here will crash when SIOCINQ is
invoked on an MPTCP socket attached to a BPF sockmap.

>  	sk_psock_put(sk, psock);
>  	unlock_sock_fast(sk, slow);
>  
> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index 7ecafbf51fa05..f6fd610f70778 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c
> @@ -398,6 +398,7 @@ static void mptcp_bpf_rebuild_protos(struct proto prot[MPTCP_BPF_NUM_CFGS],
>  	prot[MPTCP_BPF_BASE].close		= sock_map_close;
>  	prot[MPTCP_BPF_BASE].recvmsg		= mptcp_bpf_recvmsg;
>  	prot[MPTCP_BPF_BASE].sock_is_readable	= sk_msg_is_readable;
> +	prot[MPTCP_BPF_BASE].ioctl		= tcp_bpf_ioctl;

[Severity: Critical]
Since tcp_bpf_ioctl is wired into the MPTCP BPF protocol structure here,
does this expose MPTCP sockets to the NULL pointer dereference mentioned
above?

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