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

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

Pre-existing issues:
- [High] Use-After-Free (UAF) of `struct sock` on bridging and TC redirect paths due to non-refcounted sockets.
--

--- Patch [1]: [PATCH net v2 1/1] ip: orphan prefetched skbs before multicast forwarding ---
commit 8d7ee5d5781c74533a99f0b7cf5fb3db935e55f5
Author: Zhiling Zou <[email protected]>

ip: orphan prefetched skbs before multicast forwarding

This patch adds skb_orphan() calls before multicast forwarding in both
IPv4 and IPv6 to clear non-refcounted sockets assigned by bpf_sk_assign().
This prevents a use-after-free when the socket is destroyed before the
forwarded skb is freed.

Link: https://lore.kernel.org/all/02db4590d0161e31a789dcdfa8d1be1a3212ec2e.1785724784.git.zhilinz@nebusec.ai/

> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> index 1d9a4ac14fcef..e5f2b1c6150d2 100644
> --- a/net/ipv4/ipmr.c
> +++ b/net/ipv4/ipmr.c
> @@ -2213,6 +2213,9 @@ int ip_mr_input(struct sk_buff *skb)
>  	if (IPCB(skb)->flags & IPSKB_FORWARDED)
>  		goto dont_forward;
>  
> +	if (!local)
> +		skb_orphan(skb);
> +

[Severity: High]
This isn't a bug introduced by this patch, but since you are fixing the
use-after-free for multicast forwarding, do we have the exact same problem
in the TC redirect and bridge forwarding paths?

If a packet receives a non-refcounted socket via bpf_sk_assign() at TC
ingress, and the eBPF program returns TC_ACT_REDIRECT, it is redirected
to an egress qdisc:

net/core/filter.c:skb_do_redirect() {
    ...
    return __bpf_redirect(skb, dev, flags);
}

Alternatively, if it returns TC_ACT_OK and is processed by a bridge port,
it gets forwarded to an egress qdisc:

net/bridge/br_forward.c:br_forward() {
    ...
    dev_queue_xmit(skb);
}

In both paths, skb_orphan() is not called, so the SKB retains the socket
pointer while sitting asynchronously in the qdisc. If the socket's RCU grace
period expires while queued, wouldn't a later free cause the exact same
use-after-free when the destructor dereferences the freed skb->sk?

net/core/sock.c:sock_pfree() {
    struct sock *sk = skb->sk;

    if (!sk_is_refcounted(sk))
        return;
    ...
}

Should skb_orphan() be called before enqueuing to egress qdiscs in these
paths as well?

>  	mrt = ipmr_rt_fib_lookup(net, skb);
>  	if (IS_ERR(mrt)) {
>  		kfree_skb(skb);

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