Re: [PATCH net] bpf: Fix skb_pull() BUG in __bpf_redirect_neigh()

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 · Medium: 0 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The new `skb->mac_len < ETH_HLEN` test in __bpf_redirect_neigh()
  (net/core/filter.c:2439) breaks bpf_redirect_neigh() for locally…
- [High] The same short-L2 skb from the commit message still hits the
  __skb_pull() BUG if the program calls bpf_redirect() instead of…

Pre-existing issues:
- [Low] Concern: after pulling the Ethernet header,
  __bpf_redirect_neigh_v4()/__bpf_redirect_neigh_v6() read…

--

bpf: Fix skb_pull() BUG in __bpf_redirect_neigh()

This patch adds a check to __bpf_redirect_neigh() that drops skbs whose
mac_len is shorter than ETH_HLEN. It also calls pskb_may_pull() so the
Ethernet header is linear before it is read and pulled. The goal is to
avoid the __skb_pull() BUG that a TC program can trigger with
bpf_skb_change_head(skb, 1, 0) after vlan pop_eth.

Link: https://lore.kernel.org/netdev/20261027082604.1485903-1-edumazet@kernel.org/

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 70dc621672f2e..4d344ec097361 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2432,10 +2432,14 @@ static int __bpf_redirect_neigh_v4(struct sk_buff *skb, struct net_device *dev,
>  static int __bpf_redirect_neigh(struct sk_buff *skb, struct net_device *dev,
>  				struct bpf_nh_params *nh)
>  {
> -	struct ethhdr *ethh = eth_hdr(skb);
> +	struct ethhdr *ethh;
>  
>  	if (unlikely(skb->mac_header >= skb->network_header))
>  		goto out;
> +	if (unlikely(skb->mac_len < ETH_HLEN ||
> +		     !pskb_may_pull(skb, ETH_HLEN)))
> +		goto out;

[Severity: High]
Can this check drop valid locally generated packets when
bpf_redirect_neigh() is called from TC egress (clsact or tcx)?

As far as I can tell, skb->mac_len is only set on the receive path, by
skb_reset_mac_len() in __netif_receive_skb_core() and by a few BPF helpers.
The transmit path never sets it. __dev_queue_xmit() only does:

    skb_reset_mac_header(skb);

The IPv4/IPv6 output, neighbour and eth code don't touch mac_len either.
That means a locally created skb still has mac_len == 0 from allocation.
Clones, such as TCP retransmit clones, inherit that 0 through C(mac_len) in
__skb_clone().

Such an skb has a valid 14 byte linear Ethernet header. With this patch it
still takes the goto out path, gets freed, and -ENOTSUPP is returned. The
return value is then ignored in sch_handle_egress():

	case TC_ACT_REDIRECT:
		/* No need to push/pop skb's mac_header here on egress! */
		skb_do_redirect(skb);
		*ret = NET_XMIT_SUCCESS;

So the packet is dropped silently and the sender is told the transmit
succeeded. Forwarded packets still pass the check because the RX path set
mac_len to 14.

Other TC code already avoids mac_len at egress. tcf_mirred_to_dev() does:

		mac_len = at_ingress ? skb->mac_len :
			  skb_network_offset(skb);

and bpf_push_mac_rcsum() only reads mac_len when skb_at_tc_ingress() is
true.

skb->data == mac_header on both paths here. Would checking
skb_network_offset(skb) < ETH_HLEN (or skb_mac_header_len(skb)) work
instead? Another option is to use pskb_may_pull(skb, ETH_HLEN) together
with the mac_header/network_header distance, and not use the cached
mac_len at all.

The existing selftests (test_tc_neigh*.c, test_tc_dtime.c) only call this
helper from ingress, so they would not catch this.

> +	ethh = eth_hdr(skb);
>  	bpf_push_mac_rcsum(skb);
>  	if (is_multicast_ether_addr(ethh->h_dest))
>  		goto out;

[Severity: High]
Can the same short L2 header BUG still be hit through bpf_redirect()?

__bpf_redirect_common() has the same weak check that the commit message
calls insufficient:

net/core/filter.c:__bpf_redirect_common() {
	/* Verify that a link layer header is carried */
	if (unlikely(skb->mac_header >= skb->network_header || skb->len == 0)) {
		kfree_skb(skb);
		return -ERANGE;
	}
	...
}

Take the skb from the commit message (mac_len == 1, one linear byte,
skb->len >= 14). If the program calls bpf_redirect(ifindex, BPF_F_INGRESS)
in place of bpf_redirect_neigh(), the path looks like this:

skb_do_redirect()
  __bpf_redirect()
    __bpf_redirect_common()
      __bpf_rx_skb()
        dev_forward_skb_nomtu()
          __dev_forward_skb2()
            eth_type_trans()
              eth_skb_pull_mac()
                skb_pull_inline(skb, ETH_HLEN)
                  __skb_pull()  -> BUG()

____dev_forward_skb() only calls skb_orphan_frags() and
__is_skb_forwardable(), and neither of those linearizes the skb. Without
BPF_F_INGRESS, the egress path to lo or veth reaches eth_type_trans() in
the driver's xmit routine the same way.

If the linked series from Eric already covers __bpf_redirect_common() or
eth_type_trans(), it would handle this case. In this tree, though, neither
__bpf_redirect_common() nor eth_skb_pull_mac() has such a guard.

Should __bpf_redirect_common() get the same pskb_may_pull(skb, ETH_HLEN)
check?

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch. After
the Ethernet pull, __bpf_redirect_neigh() dispatches on skb->protocol
without checking how much data is left:

	if (skb->protocol == htons(ETH_P_IP))
		return __bpf_redirect_neigh_v4(skb, dev, nh);
	else if (skb->protocol == htons(ETH_P_IPV6))
		return __bpf_redirect_neigh_v6(skb, dev, nh);

__bpf_redirect_neigh_v4() and __bpf_redirect_neigh_v6() then read header
fields to build the flow key, without pskb_may_pull(skb,
sizeof(struct iphdr)) or the ipv6hdr equivalent:

net/core/filter.c:__bpf_redirect_neigh_v4() {
	const struct iphdr *ip4h = ip_hdr(skb);
	...
		.flowi4_proto = ip4h->protocol,
		.daddr	      = ip4h->daddr,
		.saddr	      = ip4h->saddr,
	...
}

If a program trims the skb down to only the L2 header, or the L3 header
sits in frags, can these reads go past skb->tail?

The reads appear to stay inside the head allocation (tailroom plus
skb_shared_info). So the result would be a garbage route/neighbour lookup,
not a memory safety problem.

Would it make sense to add an L3 pskb_may_pull() while hardening this path?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/bpf_redirect_neigh_short_linear.v1.r0.20261028033717-1%40mojatatu.com