Netdev List
 help / color / mirror / Atom feed
* [PATCH net v2] bpf: Fix skb_pull() BUG in __bpf_redirect_neigh()
@ 2026-10-29  9:36 Jamal Hadi Salim
  2026-10-30 21:47 ` Alexei Starovoitov
  0 siblings, 1 reply; 3+ messages in thread
From: Jamal Hadi Salim @ 2026-10-29  9:36 UTC (permalink / raw)
  To: netdev
  Cc: Jamal Hadi Salim, Eric Dumazet, bpf, bot+bpf-ci, Daniel Borkmann,
	Alexei Starovoitov, David Ahern, Martin KaFai Lau,
	Martin KaFai Lau, John Fastabend, Stanislav Fomichev,
	Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi,
	Song Liu, Yonghong Song, Jiri Olsa, Emil Tsalapatis,
	Ihor Solodrai, David S . Miller, Jakub Kicinski, Paolo Abeni,
	Simon Horman, Jiri Pirko, Victor Nogueira, linux-kselftest,
	Shuah Khan, stable

The issue:
This is a follow-up to the short link-layer header fixes from Eric.
After vlan pop_eth, a privileged SCHED_ACT BPF program can call
bpf_skb_change_head(skb, 1, 0) and then bpf_redirect_neigh().

__bpf_redirect_neigh() calls skb_pull(skb, sizeof(*ethh)) after checking
only that mac_header < network_header.  It does not check that the MAC
header is long enough or that it is linear.

A SCHED_ACT BPF program can do:

    bpf_skb_change_head(skb, 1, 0);
    bpf_redirect_neigh(lo, NULL, 0, 0);

after vlan pop_eth.  The skb then has mac_len == 1 and only one byte of
linear data; skb_pull(ETH_HLEN) trips the skb->len < skb->data_len BUG in
__skb_pull():

    kernel BUG at include/linux/skbuff.h:2839
    RIP: skb_pull+0xc5/0x100
    Call Trace: skb_do_redirect -> ...

The Fix:
Use skb_mac_header_len(), i.e. network_header - mac_header, rather than
skb->mac_len.  mac_len is zero for locally generated tc egress and netkit
packets, while skb_mac_header_len() still rejects the one-byte header
produced by pop_eth + bpf_skb_change_head(skb, 1, 0).

Also use pskb_may_pull() to ensure the Ethernet header is linear before
reading or pulling it, and re-read ethh afterwards since pskb_may_pull()
may reallocate skb->head.

Add egress coverage to tc_redirect_neigh: the existing test only attaches
redirect_neigh at ingress, where mac_len is populated.

Conditions to recreate the bug:
On x86-64 with 4 KiB pages, enable CONFIG_PACKET, CONFIG_BPF_SYSCALL,
CONFIG_NET_SCH_INGRESS, CONFIG_NET_CLS_MATCHALL, CONFIG_NET_ACT_VLAN and
CONFIG_NET_ACT_BPF.  Run as root with BPF loading, network administration
and raw-packet privileges.  Bring lo up and attach a SCHED_ACT program
that calls bpf_skb_change_head(skb, 1, 0), then returns
bpf_redirect_neigh(skb->ifindex, NULL, 0, 0):

  ip link set lo up
  tc qdisc add dev lo clsact
  tc filter add dev lo egress matchall action vlan pop_eth pipe \
      action bpf object-file neigh_chghead.o section action

Send an untagged ETH_P_IP frame of ETH_HLEN + 8192 bytes through an
AF_PACKET/SOCK_RAW socket bound to lo, without PACKET_VNET_HDR or
PACKET_QDISC_BYPASS.  Use a unicast destination MAC.  The sender's skb
has a 14-byte linear Ethernet prefix and a fragmented payload; pop_eth
removes that prefix and change_head restores only one linear byte.
The unfixed neighbor redirect then tries to pull 14 bytes and BUGs.

Fixes: b4ab31414970 ("bpf: Add redirect_neigh helper as redirect drop-in")
Reported-by: BPF CI <bot+bpf-ci@kernel.org>
Link: https://lore.kernel.org/all/2a0f5ff1d8db81b07fc4b344b8e68e3505fcbf36f72a471d1678b2f1cecd881b@mail.kernel.org/
Link: https://lore.kernel.org/netdev/20261027082604.1485903-1-edumazet@kernel.org/
Cc: stable@vger.kernel.org
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
v2:
- use skb_mac_header_len() instead of mac_len; mac_len is zero for
  locally generated tc egress and netkit packets (BPF CI)
- add tc-egress coverage to tc_redirect_neigh
- document the nonlinear AF_PACKET reproduction conditions

 net/core/filter.c                             |  6 ++++-
 .../selftests/bpf/prog_tests/tc_redirect.c    | 26 +++++++++++++++++++
 .../selftests/bpf/progs/test_tc_neigh.c       | 10 +++++++
 3 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 70dc621672f2..4925f3e355d4 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;
+	ethh = eth_hdr(skb);
 	bpf_push_mac_rcsum(skb);
 	if (is_multicast_ether_addr(ethh->h_dest))
 		goto out;
diff --git a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
index af8968b89ad7..829f84fb64a3 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;
+fail:
+	close_netns(nstoken);
+	return -1;
+}
+
 static struct bpf_link *netns_attach_nk(const char *ns, int ifindex,
 					struct bpf_program *prog)
 {
@@ -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;
 
diff --git a/tools/testing/selftests/bpf/progs/test_tc_neigh.c b/tools/testing/selftests/bpf/progs/test_tc_neigh.c
index de15155f2609..e0b6c14a3935 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);
+}
+
 char __license[] SEC("license") = "GPL";
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-10-31 13:32 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-29  9:36 [PATCH net v2] bpf: Fix skb_pull() BUG in __bpf_redirect_neigh() Jamal Hadi Salim
2026-10-30 21:47 ` Alexei Starovoitov
2026-10-31 13:32   ` Jamal Hadi Salim

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox