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

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

[Jamal Hadi Salim]

The issue:
This is a follow-up to the short link-layer header fixes[Link].  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:
Drop skbs whose MAC header is shorter than ETH_HLEN, and use
pskb_may_pull() to ensure the Ethernet header is linear before reading or
pulling it.  Re-read ethh after pskb_may_pull(), which may reallocate
skb->head.

Fixes: b4ab31414970 ("bpf: Add redirect_neigh helper as redirect drop-in")
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>
---
 net/core/filter.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 70dc621672f2..4d344ec09736 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;
+	ethh = eth_hdr(skb);
 	bpf_push_mac_rcsum(skb);
 	if (is_multicast_ether_addr(ethh->h_dest))
 		goto out;
-- 
2.43.0

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

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 3853 bytes --]

> bpf: Fix skb_pull() BUG in __bpf_redirect_neigh()

TL;DR: The new skb->mac_len < ETH_HLEN check in __bpf_redirect_neigh() may
wrongly drop valid packets on tc egress and netkit, where mac_len is often 0;
a regression that would reach stable.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index b7ac145b68b1..95a8a728a71d 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c

[ ... ]

> @@ -2451,10 +2451,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;

Does the new skb->mac_len < ETH_HLEN test drop valid packets on tc egress
and netkit?

skb->mac_len is only set on the receive path, where
__netif_receive_skb_core() calls skb_reset_mac_len().  The transmit path
only calls skb_reset_mac_header() in __dev_queue_xmit(), and nothing in
ip_finish_output2(), neigh_output(), eth_header(), sch_handle_egress(),
tcx_run(), cls_bpf_classify() (egress branch), netkit_xmit() or
netkit_prep_forward() writes mac_len.

A packet built by a local socket starts with mac_len == 0, because
skbuff_clear() zeroes everything before 'tail', and skb_clone() (used by TCP
transmit) copies that 0.

For example, with tc egress and a locally generated IPv4/IPv6 packet:

ip_finish_output2()
  neigh_output()
    dev_queue_xmit()
      __dev_queue_xmit()          <- skb_reset_mac_header() only
        sch_handle_egress()
          tc_run() / tcx_run()
            prog returns bpf_redirect_neigh() = TC_ACT_REDIRECT
              skb_do_redirect()
                __bpf_redirect_neigh()

Here mac_header < network_header passes, because an Ethernet header was
pushed, but mac_len == 0 < ETH_HLEN, so the skb goes to 'out', gets
kfree_skb() and -ENOTSUPP is returned.

The same happens with netkit:

netkit_xmit()
  netkit_run()
    NETKIT_REDIRECT
      skb_do_redirect()
        __bpf_redirect_neigh()

with mac_len still 0.  Netkit programs are BPF_PROG_TYPE_SCHED_CLS, and
tc_cls_act_func_proto() allows BPF_FUNC_redirect_neigh.  Forwarded traffic
that arrived on a layer 3 device (mac_len 0 on receive) and leaves through
an Ethernet device's tc egress would be dropped as well.

Before this patch these paths worked, since the Ethernet header is linear
(dev_hard_header() pushes it into the head) and skb_pull(skb, ETH_HLEN) was
fine.  bpf_out_neigh_v4/v6() calling sock_confirm_neigh(skb, neigh) also
suggests the helper is meant to handle locally generated skbs.

Elsewhere the kernel already treats mac_len as unreliable at egress.
act_mirred uses:

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

and act_mpls uses skb_network_offset(skb) when not at ingress.

The bpf_redirect_neigh selftests (test_tc_neigh, test_tc_neigh_fib,
test_tc_dtime) all attach at tc ingress, where mac_len is valid, so they
would not catch this.

Would testing the real MAC header length work instead, for example
skb_mac_header_len(skb) < ETH_HLEN (network_header - mac_header), or
skb_network_offset(skb) < ETH_HLEN with skb->data at the MAC header here?

That would still reject the 1-byte header from the commit message (pop_eth
followed by bpf_skb_change_head(skb, 1, 0)) without dropping egress and
netkit traffic.  Since the patch carries Cc: stable, this regression would
also reach stable kernels.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/40666397744

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

[Eric Dumazet]

Le mer. 28 oct. 2026 à 11:19, <bot+bpf-ci@kernel.org> a écrit :
>
> > bpf: Fix skb_pull() BUG in __bpf_redirect_neigh()
>
> TL;DR: The new skb->mac_len < ETH_HLEN check in __bpf_redirect_neigh() may
> wrongly drop valid packets on tc egress and netkit, where mac_len is often 0;
> a regression that would reach stable.
>
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index b7ac145b68b1..95a8a728a71d 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
>
> [ ... ]
>
> > @@ -2451,10 +2451,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;
>
> Does the new skb->mac_len < ETH_HLEN test drop valid packets on tc egress
> and netkit?
>


The bot is right, sorry Jamal, I missed this when looking at your
first version.

skb->mac_len is only set on the receive path. On tc egress (and netkit),
it is usually 0 for locally generated packets, so this would drop all
bpf_redirect_neigh() traffic from egress programs.

In both cases skb->data is at the MAC header when __bpf_redirect_neigh()
runs (sch_handle_ingress() pushes skb->mac_len before skb_do_redirect()),
so you can use skb_network_offset() instead:

if (unlikely(skb_network_offset(skb) < ETH_HLEN ||
     !pskb_may_pull(skb, ETH_HLEN)))
        goto out;

This still rejects the 1-byte header from bpf_skb_change_head(skb, 1, 0),
and also covers the existing mac_header >= network_header test.


> skb->mac_len is only set on the receive path, where
> __netif_receive_skb_core() calls skb_reset_mac_len().  The transmit path
> only calls skb_reset_mac_header() in __dev_queue_xmit(), and nothing in
> ip_finish_output2(), neigh_output(), eth_header(), sch_handle_egress(),
> tcx_run(), cls_bpf_classify() (egress branch), netkit_xmit() or
> netkit_prep_forward() writes mac_len.
>
> A packet built by a local socket starts with mac_len == 0, because
> skbuff_clear() zeroes everything before 'tail', and skb_clone() (used by TCP
> transmit) copies that 0.
>
> For example, with tc egress and a locally generated IPv4/IPv6 packet:
>
> ip_finish_output2()
>   neigh_output()
>     dev_queue_xmit()
>       __dev_queue_xmit()          <- skb_reset_mac_header() only
>         sch_handle_egress()
>           tc_run() / tcx_run()
>             prog returns bpf_redirect_neigh() = TC_ACT_REDIRECT
>               skb_do_redirect()
>                 __bpf_redirect_neigh()
>
> Here mac_header < network_header passes, because an Ethernet header was
> pushed, but mac_len == 0 < ETH_HLEN, so the skb goes to 'out', gets
> kfree_skb() and -ENOTSUPP is returned.
>
> The same happens with netkit:
>
> netkit_xmit()
>   netkit_run()
>     NETKIT_REDIRECT
>       skb_do_redirect()
>         __bpf_redirect_neigh()
>
> with mac_len still 0.  Netkit programs are BPF_PROG_TYPE_SCHED_CLS, and
> tc_cls_act_func_proto() allows BPF_FUNC_redirect_neigh.  Forwarded traffic
> that arrived on a layer 3 device (mac_len 0 on receive) and leaves through
> an Ethernet device's tc egress would be dropped as well.
>
> Before this patch these paths worked, since the Ethernet header is linear
> (dev_hard_header() pushes it into the head) and skb_pull(skb, ETH_HLEN) was
> fine.  bpf_out_neigh_v4/v6() calling sock_confirm_neigh(skb, neigh) also
> suggests the helper is meant to handle locally generated skbs.
>
> Elsewhere the kernel already treats mac_len as unreliable at egress.
> act_mirred uses:
>
>     mac_len = at_ingress ? skb->mac_len : skb_network_offset(skb);
>
> and act_mpls uses skb_network_offset(skb) when not at ingress.
>
> The bpf_redirect_neigh selftests (test_tc_neigh, test_tc_neigh_fib,
> test_tc_dtime) all attach at tc ingress, where mac_len is valid, so they
> would not catch this.
>
> Would testing the real MAC header length work instead, for example
> skb_mac_header_len(skb) < ETH_HLEN (network_header - mac_header), or
> skb_network_offset(skb) < ETH_HLEN with skb->data at the MAC header here?
>
> That would still reject the 1-byte header from the commit message (pop_eth
> followed by bpf_skb_change_head(skb, 1, 0)) without dropping egress and
> netkit traffic.  Since the patch carries Cc: stable, this regression would
> also reach stable kernels.
>
>
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/40666397744

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

[Jamal Hadi Salim]

On Wed, Oct 28, 2026 at 7:40 AM Eric Dumazet <edumazet@kernel.org> wrote:
>
> Le mer. 28 oct. 2026 à 11:19, <bot+bpf-ci@kernel.org> a écrit :
> >
> > > bpf: Fix skb_pull() BUG in __bpf_redirect_neigh()
> >
> > TL;DR: The new skb->mac_len < ETH_HLEN check in __bpf_redirect_neigh() may
> > wrongly drop valid packets on tc egress and netkit, where mac_len is often 0;
> > a regression that would reach stable.
> >
> > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > index b7ac145b68b1..95a8a728a71d 100644
> > > --- a/net/core/filter.c
> > > +++ b/net/core/filter.c
> >
> > [ ... ]
> >
> > > @@ -2451,10 +2451,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;
> >
> > Does the new skb->mac_len < ETH_HLEN test drop valid packets on tc egress
> > and netkit?
> >
>
>
> The bot is right, sorry Jamal, I missed this when looking at your
> first version.
>

agreed - i will send a v2. I will also test egress/netkit path.
That was fast! (in 7 days on the sashiko nipa i dont remember the
context given the number of fixes)

cheers,
jamal

> skb->mac_len is only set on the receive path. On tc egress (and netkit),
> it is usually 0 for locally generated packets, so this would drop all
> bpf_redirect_neigh() traffic from egress programs.
>
> In both cases skb->data is at the MAC header when __bpf_redirect_neigh()
> runs (sch_handle_ingress() pushes skb->mac_len before skb_do_redirect()),
> so you can use skb_network_offset() instead:
>
> if (unlikely(skb_network_offset(skb) < ETH_HLEN ||
>      !pskb_may_pull(skb, ETH_HLEN)))
>         goto out;
>
> This still rejects the 1-byte header from bpf_skb_change_head(skb, 1, 0),
> and also covers the existing mac_header >= network_header test.
>
>
> > skb->mac_len is only set on the receive path, where
> > __netif_receive_skb_core() calls skb_reset_mac_len().  The transmit path
> > only calls skb_reset_mac_header() in __dev_queue_xmit(), and nothing in
> > ip_finish_output2(), neigh_output(), eth_header(), sch_handle_egress(),
> > tcx_run(), cls_bpf_classify() (egress branch), netkit_xmit() or
> > netkit_prep_forward() writes mac_len.
> >
> > A packet built by a local socket starts with mac_len == 0, because
> > skbuff_clear() zeroes everything before 'tail', and skb_clone() (used by TCP
> > transmit) copies that 0.
> >
> > For example, with tc egress and a locally generated IPv4/IPv6 packet:
> >
> > ip_finish_output2()
> >   neigh_output()
> >     dev_queue_xmit()
> >       __dev_queue_xmit()          <- skb_reset_mac_header() only
> >         sch_handle_egress()
> >           tc_run() / tcx_run()
> >             prog returns bpf_redirect_neigh() = TC_ACT_REDIRECT
> >               skb_do_redirect()
> >                 __bpf_redirect_neigh()
> >
> > Here mac_header < network_header passes, because an Ethernet header was
> > pushed, but mac_len == 0 < ETH_HLEN, so the skb goes to 'out', gets
> > kfree_skb() and -ENOTSUPP is returned.
> >
> > The same happens with netkit:
> >
> > netkit_xmit()
> >   netkit_run()
> >     NETKIT_REDIRECT
> >       skb_do_redirect()
> >         __bpf_redirect_neigh()
> >
> > with mac_len still 0.  Netkit programs are BPF_PROG_TYPE_SCHED_CLS, and
> > tc_cls_act_func_proto() allows BPF_FUNC_redirect_neigh.  Forwarded traffic
> > that arrived on a layer 3 device (mac_len 0 on receive) and leaves through
> > an Ethernet device's tc egress would be dropped as well.
> >
> > Before this patch these paths worked, since the Ethernet header is linear
> > (dev_hard_header() pushes it into the head) and skb_pull(skb, ETH_HLEN) was
> > fine.  bpf_out_neigh_v4/v6() calling sock_confirm_neigh(skb, neigh) also
> > suggests the helper is meant to handle locally generated skbs.
> >
> > Elsewhere the kernel already treats mac_len as unreliable at egress.
> > act_mirred uses:
> >
> >     mac_len = at_ingress ? skb->mac_len : skb_network_offset(skb);
> >
> > and act_mpls uses skb_network_offset(skb) when not at ingress.
> >
> > The bpf_redirect_neigh selftests (test_tc_neigh, test_tc_neigh_fib,
> > test_tc_dtime) all attach at tc ingress, where mac_len is valid, so they
> > would not catch this.
> >
> > Would testing the real MAC header length work instead, for example
> > skb_mac_header_len(skb) < ETH_HLEN (network_header - mac_header), or
> > skb_network_offset(skb) < ETH_HLEN with skb->data at the MAC header here?
> >
> > That would still reject the 1-byte header from the commit message (pop_eth
> > followed by bpf_skb_change_head(skb, 1, 0)) without dropping egress and
> > netkit traffic.  Since the patch carries Cc: stable, this regression would
> > also reach stable kernels.
> >
> >
> > ---
> > AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> > See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
> >
> > CI run summary: https://github.com/kernel-patches/bpf/actions/runs/40666397744

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

[Jamal Hadi Salim]

On Wed, Oct 28, 2026 at 5:19 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> The issue:
> This is a follow-up to the short link-layer header fixes[Link].  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:
> Drop skbs whose MAC header is shorter than ETH_HLEN, and use
> pskb_may_pull() to ensure the Ethernet header is linear before reading or
> pulling it.  Re-read ethh after pskb_may_pull(), which may reallocate
> skb->head.
>

pw-bot: cr


cheers,
jamal

> Fixes: b4ab31414970 ("bpf: Add redirect_neigh helper as redirect drop-in")
> 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>
> ---
>  net/core/filter.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 70dc621672f2..4d344ec09736 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;
> +       ethh = eth_hdr(skb);
>         bpf_push_mac_rcsum(skb);
>         if (is_multicast_ether_addr(ethh->h_dest))
>                 goto out;
> --
> 2.43.0
>

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

[Daniel Borkmann]

On 10/28/26 12:53 PM, Jamal Hadi Salim wrote:
> On Wed, Oct 28, 2026 at 7:40 AM Eric Dumazet <edumazet@kernel.org> wrote:
>> Le mer. 28 oct. 2026 à 11:19, <bot+bpf-ci@kernel.org> a écrit :
>>>
>>>> bpf: Fix skb_pull() BUG in __bpf_redirect_neigh()
>>>
>>> TL;DR: The new skb->mac_len < ETH_HLEN check in __bpf_redirect_neigh() may
>>> wrongly drop valid packets on tc egress and netkit, where mac_len is often 0;
>>> a regression that would reach stable.
>>>
>>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>>> index b7ac145b68b1..95a8a728a71d 100644
>>>> --- a/net/core/filter.c
>>>> +++ b/net/core/filter.c
>>>
>>> [ ... ]
>>>
>>>> @@ -2451,10 +2451,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;
>>>
>>> Does the new skb->mac_len < ETH_HLEN test drop valid packets on tc egress
>>> and netkit?
>>
>> The bot is right, sorry Jamal, I missed this when looking at your
>> first version.
> 
> agreed - i will send a v2. I will also test egress/netkit path.
> That was fast! (in 7 days on the sashiko nipa i dont remember the
> context given the number of fixes)
Lets also add a BPF selftest for all corner cases to make sure things don't break.
If you need help, I can also take your fix, add a netkit selftest on top and then
we ship via bpf.

Thanks,
Daniel

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

[netdev-bot+sashiko]

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