Re: [PATCH bpf-next v4 4/6] sockmap: Fix use-after-free in udp_bpf_recvmsg()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Infinite tight loop (soft lockup) holding lock_sock if data is pending only in ingress_skb.
- [High] Permanent bricking of socket receive path in `__sk_msg_recvmsg` on 0-byte datagrams or failed copies.
- [High] Delegating blocking recvmsg to `sk_udp_recvmsg` misses BPF wakeups, causing indefinite hangs.
- [High] `vsock_msg_wait_data` sleeps while holding `lock_sock(sk)`, leading to a deadlock.
--

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

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

syzbot reported use-after-free of struct sk_msg in sk_msg_recvmsg().
sk_msg_recvmsg() peeks sk_msg from psock->ingress_msg under a lock,
but its processing is lockless.

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
> --- a/net/ipv4/udp_bpf.c
> +++ b/net/ipv4/udp_bpf.c

[ ... ]

>  static int udp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>  			   int flags)
>  {
>  	struct sk_psock *psock;
>  	int copied, ret;

[ ... ]

>  	psock = sk_psock_get(sk);
>  	if (unlikely(!psock))
>  		return sk_udp_recvmsg(sk, msg, len, flags);
>  
>  	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 a blocking recvmsg to
sk_udp_recvmsg here cause us to miss BPF wakeups?

If psock_has_data(psock) is initially false, udp_bpf_recvmsg() bypasses its
BPF-aware wait loop and delegates the read entirely to the native
sk_udp_recvmsg().

If the socket is blocking, sk_udp_recvmsg() sleeps waiting for packets on
sk->sk_receive_queue. If a BPF program subsequently redirects data to the
socket, the data is placed in psock->ingress_skb or psock->ingress_msg, and
a wakeup is issued via sk_data_ready().

This wakes up the native UDP loop, which only checks sk->sk_receive_queue,
finds it empty, and goes back to sleep. Can this cause the process to hang
indefinitely despite data being correctly available in the psock?

> +	lock_sock(sk);
>  msg_bytes_ready:
>  	copied = sk_msg_recvmsg(sk, psock, msg, len, flags);
>  	if (!copied) {
> @@ -90,11 +93,17 @@ static int udp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>  		if (data) {
>  			if (psock_has_data(psock))
>  				goto msg_bytes_ready;

[Severity: High]
This is a pre-existing issue, but can this loop cause a soft lockup if data
is pending only in ingress_skb?

If data has been enqueued to psock->ingress_skb but not yet moved to
psock->ingress_msg by the backlog worker, sk_msg_recvmsg() returns 0.
The code then calls udp_msg_wait_data().

Since psock_has_data() evaluates to true when ingress_skb is not empty,
udp_msg_wait_data() returns 1 immediately without sleeping. The loop then
jumps back to msg_bytes_ready and repeats.

On a non-preemptible kernel, if the worker is scheduled on the same CPU,
could this starve the worker and form a busy-wait loop in kernel space?


While reviewing the rx paths associated with this change, I noticed a
couple of other related issues in the subsystem.

> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c

[ ... ]

>  static int __sk_msg_recvmsg(struct sock *sk, struct sk_psock *psock,
>  			    struct msghdr *msg, int len, int flags,
>  			    struct sk_msg *msg_rx)
>  {

[ ... ]

> 			if (!copy) {
> 				copied = copied ? copied : -EFAULT;
> 				goto out;
> 			}

[Severity: High]
This is a pre-existing issue, but does returning -EFAULT here permanently
brick the socket receive path for 0-byte datagrams?

When a 0-byte UDP packet is redirected, skb_to_sgvec yields an empty
scatterlist, making copy evaluate to 0.

Because the error path jumps to out, it bypasses advancing the read pointer
(msg_rx->sg.start = i) and skips dequeuing the message.

On the next recvmsg call, won't the function peek the same 0-byte message
and immediately return -EFAULT again?


> diff --git a/net/vmw_vsock/vsock_bpf.c b/net/vmw_vsock/vsock_bpf.c
> --- a/net/vmw_vsock/vsock_bpf.c
> +++ b/net/vmw_vsock/vsock_bpf.c

[ ... ]

>  static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
>  			     size_t len, int flags)
>  {
>  	struct sk_psock *psock;

[ ... ]

> 	lock_sock(sk);
> 	vsk = vsock_sk(sk);
> ...
> 	while (copied == 0) {
> 		long timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
>
> 		if (!vsock_msg_wait_data(sk, psock, timeo)) {

[Severity: High]
This is a pre-existing issue, but does vsock_msg_wait_data sleep while
holding lock_sock(sk)?

In vsock_bpf_recvmsg(), the socket lock is held when calling
vsock_msg_wait_data(). Inside vsock_msg_wait_data(), wait_woken() is called
to sleep without releasing the socket lock.

Meanwhile, the virtio transport receive path (virtio_transport_recv_pkt)
must acquire lock_sock(sk) to deliver incoming packets.

Could this result in a deadlock since the transport worker is blocked trying
to acquire lock_sock(sk) while the user process sleeps holding it?

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