Re: [PATCH net v2] 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: 0 (1) · Medium: 1 (1) · Low: 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).

- [Medium] The new netns_load_egress_bpf() calls inside
  test_tc_redirect_neigh() leave the test thread in the test runner's…
- [Low] No selftest runs the new rejection branch in
  __bpf_redirect_neigh() (skb_mac_header_len(skb) < ETH_HLEN ||…

Pre-existing issues:
- [High] The patch closes the ETH_HLEN pull BUG only in
  __bpf_redirect_neigh().
- [Medium] __bpf_redirect_neigh() now linearizes only ETH_HLEN bytes,
  then does skb_pull(ETH_HLEN) and skb_reset_network_header(), and…

--

bpf: Fix skb_pull() BUG in __bpf_redirect_neigh()

This commit hardens __bpf_redirect_neigh() before it reads and pulls the
Ethernet header. The MAC header length (network_header - mac_header) must
now be at least ETH_HLEN, and pskb_may_pull() must make the header linear.
It also adds tc egress coverage to the tc_redirect_neigh selftest.

Link: https://lore.kernel.org/all/2a0f5ff1d8db81b07fc4b344b8e68e3505fcbf36f72a471d1678b2f1cecd881b@mail.kernel.org/
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..4925f3e355d46 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_header_len(skb) < ETH_HLEN ||
> +		     !pskb_may_pull(skb, ETH_HLEN)))
> +		goto out;

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. Still,
the skb from the commit message's own reproducer looks like it can BUG
through the sibling redirect paths. Is that expected?

After vlan pop_eth and bpf_skb_change_head(skb, 1, 0) on the nonlinear
AF_PACKET skb, len is 8193, data_len is 8192, and one L2 byte is linear.
__bpf_redirect_common() only checks:

net/core/filter.c:__bpf_redirect_common() {
    ...
	if (unlikely(skb->mac_header >= skb->network_header || skb->len == 0)) {
    ...
}

so the 1-byte MAC header gets through there. With BPF_F_INGRESS:

bpf_redirect(BPF_F_INGRESS)
  __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()

Without BPF_F_INGRESS the path is:

__bpf_tx_skb()
  dev_queue_xmit(lo)
    loopback_xmit()
      eth_type_trans()

veth goes through __dev_forward_skb() and ends the same way.
bpf_clone_redirect() takes the same __bpf_redirect() path. On lo, a plain
TC_ACT_OK instead of bpf_redirect_neigh() also reaches
loopback_xmit()->eth_type_trans().

In each case the 14 <= 8193 length check passes. __skb_pull() then leaves
len at 8179, which is below data_len (8192), and hits BUG().

The commit message calls this a follow-up to Eric's short link-layer
header fixes. This tree has no pskb_may_pull() in eth_type_trans() or
eth_skb_pull_mac(), and bpf_skb_change_head() still accepts a header
shorter than ETH_HLEN.

Should one of those two places be hardened as well? Otherwise stable
backporters may assume this change fully closes the reproducer.

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

[Severity: Medium]
This isn't a bug introduced by this patch, but only ETH_HLEN bytes are
made linear here. After skb_pull(skb, sizeof(*ethh)) and
skb_reset_network_header(), can the L3 header sit entirely in frags?

__bpf_redirect_neigh_v4() reads the header directly from skb->data:

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

__bpf_redirect_neigh_v6() does the same with ipv6_hdr(skb) for flowinfo,
nexthdr, saddr and daddr. When the route has no gateway,
bpf_out_neigh_v4()->ip_neigh_for_gw() reads ip_hdr(skb)->daddr again.

Take an AF_PACKET SOCK_RAW frame where prepad + len >= PAGE_SIZE.
packet_snd() and packet_alloc_skb() make only hard_header_len (14) bytes
linear. The frame passes both new checks, and skb_pull(14) leaves
skb_headlen() == 0.

Wouldn't the 20 or 40 byte L3 reads then come from past skb->tail, from
the tailroom or skb_shared_info? Those bytes would become the flowi4 or
flowi6 keys and drive the neighbour lookup. No pop_eth or change_head is
needed, just bpf_redirect_neigh() on such a packet.

Would a pskb_may_pull() of sizeof(struct iphdr) or sizeof(struct ipv6hdr)
before calling the v4/v6 helpers be appropriate?

> diff --git a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
> index af8968b89ad7c..829f84fb64a30 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
> @@ -407,6 +407,26 @@ static int netns_load_bpf(const struct bpf_program *src_prog,
>  	return -1;
>  }
>  
> +static int netns_load_egress_bpf(const struct bpf_program *prog,
> +				 const char *ns, int ifindex)
> +{
> +	LIBBPF_OPTS(bpf_tc_hook, qdisc);
> +	struct nstoken *nstoken;
> +	int err;
> +
> +	nstoken = open_netns(ns);
> +	if (!ASSERT_OK_PTR(nstoken, "setns egress"))
> +		return -1;
> +
> +	QDISC_CLSACT_CREATE(&qdisc, ifindex);
> +	XGRESS_FILTER_ADD(&qdisc, BPF_TC_EGRESS, prog, 0);
> +	close_netns(nstoken);
> +	return 0;

[ ... ]

> @@ -1055,6 +1075,12 @@ static void test_tc_redirect_neigh(struct netns_setup_result *setup_result)
>  			   skel->progs.tc_chk, setup_result))
>  		goto done;
>  
> +	if (netns_load_egress_bpf(skel->progs.tc_egress, NS_SRC,
> +				  setup_result->ifindex_src) ||
> +	    netns_load_egress_bpf(skel->progs.tc_egress, NS_DST,
> +				  setup_result->ifindex_dst))
> +		goto done;
> +
>  	if (!ASSERT_OK(set_forwarding(false), "disable forwarding"))
>  		goto done;

[Severity: Medium]
Is the test thread still in NS_FWD when set_forwarding(false) runs here?

test_tc_redirect() runs the tests in a separate pthread. open_netns()
saves the original namespace with:

tools/testing/selftests/bpf/network_helpers.c:open_netns() {
    ...
	token->orig_netns_fd = open("/proc/self/ns/net", O_RDONLY);
    ...
}

/proc/self resolves to the thread-group leader. So the saved fd points to
the runner's root netns, not NS_FWD, which the worker entered at the top
of test_tc_redirect_neigh().

The close_netns() calls in netns_load_egress_bpf() therefore setns() the
worker back into the root netns. set_forwarding(false) then writes the
ipv4 and ipv6 forwarding sysctls, and those resolve through
current->nsproxy->net_ns.

Would this turn off forwarding in the host namespace and leave NS_FWD
unchanged? Nothing restores the setting afterwards. The test would also no
longer check redirect_neigh with forwarding disabled in NS_FWD.

test_tc_redirect_peer_ing() already has the same pattern via
netns_attach_nk(). Any of these would avoid it:

- attach the egress programs before entering NS_FWD
- re-enter NS_FWD before calling set_forwarding()
- have open_netns() save /proc/thread-self/ns/net

> diff --git a/tools/testing/selftests/bpf/progs/test_tc_neigh.c b/tools/testing/selftests/bpf/progs/test_tc_neigh.c
> index de15155f2609f..e0b6c14a3935e 100644
> --- a/tools/testing/selftests/bpf/progs/test_tc_neigh.c
> +++ b/tools/testing/selftests/bpf/progs/test_tc_neigh.c
> @@ -133,4 +133,14 @@ int tc_src(struct __sk_buff *skb)
>  	return bpf_redirect_neigh(IFINDEX_DST, NULL, 0, 0);
>  }
>  
> +SEC("tc")
> +int tc_egress(struct __sk_buff *skb)
> +{
> +	if (skb->mark)
> +		return TC_ACT_OK;
> +
> +	skb->mark = 1;
> +	return bpf_redirect_neigh(skb->ifindex, NULL, 0, 0);
> +}

[Severity: Low]
This isn't a bug, but does any packet in this test reach the new rejection
branch in __bpf_redirect_neigh()?

This program sees ordinary locally generated TCP/ICMP packets at veth
egress. __dev_queue_xmit() has already reset mac_header there, so
skb_mac_header_len() is 14 and the header is linear. These packets pass
both the old and the new checks, so removing the new check would go
unnoticed.

tc_redirect_neigh is also registered only for veth:

	RUN_TEST(tc_redirect_neigh, MODE_VETH);

so the netkit case mentioned in the changelog is not exercised.

Would a negative test be worth adding? For example,
bpf_skb_change_head(skb, 1, 0) on a nonlinear skb followed by
bpf_redirect_neigh(), expecting a drop.

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