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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: packet drop on egress in __bpf_redirect_neigh() due to mac_len check
--

commit cc6241a04af2e98e3d6758d1d73c0ca8995faa4a
Author: Jamal Hadi Salim <jhs@mojatatu.com>

bpf: Fix skb_pull() BUG in __bpf_redirect_neigh()

This commit adds a check on skb->mac_len and a pskb_may_pull() call in
__bpf_redirect_neigh() before reading and pulling the Ethernet header to
prevent a BUG in __skb_pull() on short or non-linear link-layer headers.

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]
Does checking skb->mac_len < ETH_HLEN here cause __bpf_redirect_neigh() to
drop valid packets on egress and netkit transmit paths?

bpf_redirect_neigh() is available to BPF_PROG_TYPE_SCHED_CLS and
BPF_PROG_TYPE_SCHED_ACT programs via tc_cls_act_func_proto(), which run on
TC egress in sch_handle_egress() and in netkit_xmit() in addition to TC
ingress.

While the receive path sets skb->mac_len in __netif_receive_skb_core() via
skb_reset_mac_len(skb), the transmit path for locally-originated or
L3-forwarded packets leaves skb->mac_len as 0 when pushing the Ethernet
header and only resets skb->mac_header:

net/core/dev.c:__dev_queue_xmit() {
	...
	skb_reset_mac_header(skb);
	skb_assert_len(skb);
	...
	skb = sch_handle_egress(skb, &rc, dev);
}

and in drivers/net/netkit.c:

drivers/net/netkit.c:netkit_prep_forward() {
	skb_scrub_packet(skb, false);
	nf_skip_egress(skb, true);
	skb_reset_mac_header(skb);
	...
}

drivers/net/netkit.c:netkit_xmit() {
	...
	netkit_prep_forward(skb, ! net_eq(net, peer_net), xnet_scrub);
	...
	ret = netkit_run(prog, skb);
	switch (ret) {
	...
	case NETKIT_REDIRECT:
		dev_sw_netstats_tx_add(dev, 1, len);
		skb_do_redirect(skb);
		break;
}

When sch_handle_egress() or netkit_xmit() calls skb_do_redirect(skb) ->
__bpf_redirect_neigh(), skb->network_header - skb->mac_header is ETH_HLEN,
but skb->mac_len is 0. This causes skb->mac_len < ETH_HLEN to evaluate to
true, freeing the skb via kfree_skb(skb) and returning -ENOTSUPP.

Conversely, if a packet received on an Ethernet device (where skb->mac_len
was set to ETH_HLEN) is forwarded via __dev_queue_xmit() out a device with a
link-layer header shorter than ETH_HLEN, __dev_queue_xmit() updates
skb->mac_header without updating skb->mac_len. The stale skb->mac_len passes
the skb->mac_len < ETH_HLEN check and skb_pull(skb, sizeof(*ethh)) pulls
past skb->network_header into the network header.

Should __bpf_redirect_neigh() instead check that skb->mac_header + ETH_HLEN
<= skb->network_header (or skb_network_offset(skb) >= ETH_HLEN), similar to
__bpf_redirect_no_mac() and skb_eth_pop()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/bpf_redirect_neigh_short_linear.v1.r0.20261028033717-1@mojatatu.com?part=1