Re: [PATCH net v3 1/1] ip: orphan prefetched skbs before multicast forwarding

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] bpf_sk_assign on ingress paired with bpf_redirect (TC redirect) or bridge forwarding allows an SKB to escape the RCU read-side critical section without being orphaned, causing a Use-After-Free of a UDP socket.
--

--- Patch [1]: [PATCH net v3 1/1] ip: orphan prefetched skbs before multicast forwarding ---
commit 94ee963b9e52780b05e88dd7a54693c9630a26fb
Author: Zhiling Zou <zhilinz@nebusec.ai>

ip: orphan prefetched skbs before multicast forwarding

IPv4 and IPv6 input preserve an skb->sk association installed by
bpf_sk_assign() so that local delivery can use the selected socket under
RCU. Both address families can also prefetch a socket in UDP early demux.
In both paths (BPF and UDP early demux) a reference is not guaranteed to
be held on the socket. This patch orphans the skb before each non-local
multicast forwarding path.

Fixes: cf7fbe660f2d ("bpf: Add socket assign support")
Fixes: 08842c43d016 ("udp: no longer touch sk->sk_refcnt in early demux")
Cc: stable@vger.kernel.org
Reported-by: Vega <vega@nebusec.ai>
Signed-off-by: Zhiling Zou <zhilinz@nebusec.ai>

> ---
> changes in v3:
> - Describe UDP early demux consistently for both IPv4 and IPv6.
> - Drop the IPv6-specific unicast-MAC discussion from the commit message.
> - Leave the BPF-only TC redirect and bridge paths for a separate patchset.

[Severity: Critical]
This is a pre-existing issue, but since the TC redirect and bridge paths are
left for a separate patchset, is there still a risk of a use-after-free for
UDP sockets here?

If a BPF program on TC ingress uses bpf_sk_assign() to assign a socket to an
SKB without holding a reference, and then calls bpf_redirect():

net/core/filter.c:skb_do_redirect() {
    ...
    return flags & BPF_F_NEIGH ?
           __bpf_redirect_neigh(skb, dev, flags & BPF_F_NEXTHOP ?
                                &ri->nh : NULL) :
           __bpf_redirect(skb, dev, flags);
}

This pushes the SKB to the egress path via __bpf_redirect() -> __bpf_tx_skb()
-> dev_queue_xmit() without calling skb_orphan().

A similar sequence exists in bridge forwarding:

net/bridge/br_forward.c:br_forward_finish() {
    skb_clear_tstamp(skb);
    return NF_HOOK(NFPROTO_BRIDGE, NF_BR_POST_ROUTING,
                   net, sk, skb, NULL, skb->dev,
                   br_dev_queue_push_xmit);
}

This pushes the SKB to egress via br_dev_queue_push_xmit() ->
dev_queue_xmit() without orphaning it.

Does this allow the SKB to escape the RCU read-side critical section with a
dangling socket pointer if the socket is closed concurrently?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0c52eb3d7532aaf8bccf37e0f7c922143c639735.1786552223.git.zhilinz@nebusec.ai?part=1