Re: [PATCH bpf-next v3 5/7] sockmap: Fix use-after-free in udp_bpf_recvmsg()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Critical] Infinite loop holding the socket lock when data is pending in `ingress_skb` but not yet in `ingress_msg`.

Pre-existing issues:
- [High] Blocking recvmsg() incorrectly returns -EAGAIN instead of -EINTR (-ERESTARTSYS) when interrupted by a signal.
- [High] Delegating to `sk_udp_recvmsg` ignores data subsequently arriving via BPF redirect, causing a permanent hang.
- [High] udp_bpf_recvmsg() resets the receive timeout on every loop iteration, allowing indefinite blocking despite SO_RCVTIMEO.
- [High] udp_msg_wait_data() does not check sk_shutdown after waking up, causing recvmsg to return -EAGAIN instead of 0 on a closed socket.
--

commit d3eae210a4563564468c062b23ec1d89b4e36196
Author: Kuniyuki Iwashima <kuniyu@google.com>

sockmap: Fix use-after-free in udp_bpf_recvmsg()

This commit fixes a use-after-free of struct sk_msg in sk_msg_recvmsg()
reported by syzbot. It serialises sk_msg_recvmsg() by adding lock_sock()
inside udp_bpf_recvmsg(), ensuring that multiple threads do not concurrently
process the same sk_msg.

Link: https://lore.kernel.org/netdev/69922ac9.a70a0220.2c38d7.00e0.GAE@google.com/

> diff --git a/net/ipv4/udp_bpf.c b/net/ipv4/udp_bpf.c
> index 9f33b07b14813..ad57c4c9eaab6 100644
> --- a/net/ipv4/udp_bpf.c
> +++ b/net/ipv4/udp_bpf.c
> @@ -50,7 +50,9 @@ static int udp_msg_wait_data(struct sock *sk, struct sk_psock *psock,
>  	sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
>  	ret = udp_msg_has_data(sk, psock);
>  	if (!ret) {
> +		release_sock(sk);
>  		wait_woken(&wait, TASK_INTERRUPTIBLE, timeo);
> +		lock_sock(sk);
>  		ret = udp_msg_has_data(sk, psock);

[Severity: High]
This is a pre-existing issue, but does this miss checking for socket shutdown
after waking up?

If the socket is shut down (RCV_SHUTDOWN) while the thread is sleeping in
wait_woken(), the thread wakes up and evaluates udp_msg_has_data(). If it
finds no data, it returns 0.

The caller udp_bpf_recvmsg() then sets copied = -EAGAIN and returns it to
userspace. Does this violate POSIX semantics, which require a blocking
recvmsg() to return 0 (EOF) immediately on a gracefully closed socket?

[Severity: High]
This is also a pre-existing issue, but is there a missing check for
signal_pending(current) here?

If wait_woken() is interrupted by a signal, it returns early. Since
udp_msg_wait_data() fails to check for signals after the wait, it will
return 0 if no data has arrived.

In udp_bpf_recvmsg(), this 0 is unconditionally translated to -EAGAIN.
Should this instead return -EINTR / -ERESTARTSYS when interrupted by a
signal? Returning -EAGAIN on a blocking socket can cause applications
to fail or spin indefinitely.

>  	}
>  	sk_clear_bit(SOCKWQ_ASYNC_WAITDATA, sk);

[ ... ]

>  	if (!psock_has_data(psock)) {
>  		ret = sk_udp_recvmsg(sk, msg, len, flags);
>  		goto out;
>  	}

[Severity: High]
This is a pre-existing issue, but does delegating to sk_udp_recvmsg() here
ignore data that might arrive via BPF redirect?

If the socket currently has no data (!psock_has_data(psock)), the fast path
drops into sk_udp_recvmsg(). If the socket is blocking, sk_udp_recvmsg()
will block waiting for data in sk->sk_receive_queue.

If a BPF program subsequently redirects a packet to this socket's psock, the
wait queue is woken up, but sk_udp_recvmsg() only checks
sk->sk_receive_queue. Finding it empty, it will go back to sleep
indefinitely. Can this result in a permanent hang?

>  
> +	lock_sock(sk);
>  msg_bytes_ready:
>  	copied = sk_msg_recvmsg(sk, psock, msg, len, flags);
>  	if (!copied) {
>  		long timeo;
>  		int data;
>  
>  		timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);

[Severity: High]
This is a pre-existing issue, but does this reset the receive timeout on
every loop iteration?

Inside the loop, sock_rcvtimeo() is called to fetch the timeout value.
udp_msg_wait_data() uses this timeout, but drops the remaining time when it
returns.

On the next iteration (for example, due to a spurious wakeup),
sock_rcvtimeo() is called again, reloading the full original timeout from
the socket. Doesn't this break SO_RCVTIMEO semantics by allowing the socket
to block indefinitely if wakeups keep occurring?

>  		data = udp_msg_wait_data(sk, psock, timeo);
>  		if (data) {
>  			if (psock_has_data(psock))
>  				goto msg_bytes_ready;

[Severity: Critical]
Does this loop introduce a hard lockup if data is pending in the
ingress_skb queue but not yet in ingress_msg?

If data is queued in psock->ingress_skb but not yet processed into
psock->ingress_msg, psock_has_data() returns true, but sk_msg_recvmsg()
returns 0.

The code then calls udp_msg_wait_data():

static int udp_msg_wait_data(...)
{
    ...
    ret = udp_msg_has_data(sk, psock);
    if (!ret) {
        release_sock(sk);
        wait_woken(&wait, TASK_INTERRUPTIBLE, timeo);
        lock_sock(sk);
        ...
    }
    return ret;
}

Because udp_msg_has_data() evaluates to true, udp_msg_wait_data() returns 1
immediately WITHOUT releasing lock_sock(sk).

Back in udp_bpf_recvmsg(), the code sees data is non-zero and
psock_has_data() is true, so it jumps to msg_bytes_ready. Because we still
hold lock_sock(sk), no other thread can process the ingress_skb, and
sk_msg_recvmsg() will again return 0.

Can this result in a 100% CPU tight spin loop while holding the socket
lock?

> +
> +			release_sock(sk);
> +
>  			ret = sk_udp_recvmsg(sk, msg, len, flags);
>  			goto out;
>  		}
>  		copied = -EAGAIN;
>  	}
> +
> +	release_sock(sk);
> +
>  	ret = copied;
>  out:
>  	sk_psock_put(sk, psock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612130919.299124-1-jiayuan.chen@linux.dev?part=5