Re: [PATCH net v2 2/6] net/sched: act_mpls: pull the inner Ethernet header after POP

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Jamal Hadi Salim wrote:
> On Fri, Oct 23, 2026 at 7:46 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > . On Fri, Oct 23, 2026 at 6:50 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > Jamal Hadi Salim wrote:
> > > > On Fri, Oct 23, 2026 at 12:09 PM Willem de Bruijn
> > > > <willemdebruijn.kernel@gmail.com> wrote:
> > > > >
> > > > > Jamal Hadi Salim wrote:
> > > > > > The issue:
> > > > > > act_mpls POP can leave the inner Ethernet header in a page fragment after
> > > > > > skb_mpls_pop(), either on an L3 device (mac_len == 0) or chained after
> > > > > > act_vlan POP_ETH.  A following eth_type_trans() can then BUG in
> > > > > > __skb_pull().
> > > > > >
> > > > > > The Fix:
> > > > > > After the pop, if the original MAC header is shorter than ETH_HLEN, pull
> > > > > > the inner Ethernet header into the linear area and drop only when
> > > > > > pskb_may_pull(skb, ETH_HLEN) fails.  skb_mpls_pop() does nothing when the
> > > > > > skb was not MPLS, so no separate state is needed.
> > > > > >
> > > > > > Conditions to recreate the bug:
> > > > > > - egress tc filter on lo: action vlan pop_eth action mpls pop
> > > > > >   (chained), or on an L3 device where mac_len == 0.
> > > > > > - AF_PACKET SOCK_RAW with PACKET_VNET_HDR, hdr_len=28, and a non-linear
> > > > > >   eth / mpls / inner-eth frame.
> > > > > > - The inner header is in page frags; a following eth_type_trans() can BUG
> > > > > >   in __skb_pull().
> > > > > >
> > > > > > Fixes: 2a2ea50870ba ("net: sched: add mpls manipulation actions to TC")
> > > > > > Reported-by: Vega <vega@nebusec.ai>
> > > > > > Link: https://lore.kernel.org/r/40fe2d7dfce13411bfbe9271c0f7c54fb7e88e3b.1782548651.git.bronzed_45_vested@icloud.com/
> > > > > > Link: https://lore.kernel.org/netdev/CAL4WiiqSMdYQYeeW0Fk_GK6ekR4xiS4zPrGKrbB9wZknpqf1-w@mail.gmail.com/
> > > > > > Cc: stable@vger.kernel.org
> > > > > > Suggested-by: Jakub Kicinski <kuba@kernel.org>
> > > > > > Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > > > > > Suggested-by: Eric Dumazet <edumazet@kernel.org>
> > > > > > Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> > > > > > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> > > > > > ---
> > > > > >  net/sched/act_mpls.c | 2 ++
> > > > > >  1 file changed, 2 insertions(+)
> > > > > >
> > > > > > diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
> > > > > > index 4ea8b2e08c3a..5363bca892fb 100644
> > > > > > --- a/net/sched/act_mpls.c
> > > > > > +++ b/net/sched/act_mpls.c
> > > > > > @@ -79,6 +79,8 @@ TC_INDIRECT_SCOPE int tcf_mpls_act(struct sk_buff *skb,
> > > > > >               if (skb_mpls_pop(skb, p->tcfm_proto, mac_len,
> > > > > >                                skb->dev && skb->dev->type == ARPHRD_ETHER))
> > > > > >                       goto drop;
> > > > > > +             if (mac_len < ETH_HLEN && !pskb_may_pull(skb, ETH_HLEN))
> > > > > > +                     goto drop;
> > > > >
> > > > > Is this first condition needed?
> > > > >
> > > >
> > > > That was me conjecturing from Eric saying in v1 "...the check can only
> > > > matter when the original MAC header is short.."
> > > > Since i have to send a v3 (Thanks Eric;->) I will remove it
> > > >
> > > > > More importantly, can it hide a bug?
> > > > >
> > > >
> > > > What kind of bug?
> > >
> > > Two kinds. One, if original mac_len is larger, gets popped and the
> > > resultant packet is still too short for ETH_HLEN afterwards.
> > >
> > > But this can probably not happen, as skb_mpls_pop only pops one MPLS
> > > tag.
> > >
> > > What this patch tries to ensure is that the subsequent Ethernet
> > > header is linear.
> > >
> > > The second potential bug is this assumption that after the MPLS tag
> > > lies an Ethernet header. Is that a valid assumption? Or could we drop
> > > a valid packet with smaller header.
> > >
> >
> > tbh, I was thinking more about skb frags earlier.
> > You are right only ETH_P_TEB promises an inner Ethernet header; for
> > other inner protocols the pull is wrong, so I’ll check for
> > p->tcfm_proto == htons(ETH_P_TEB). I didnt have this check in the
> > current fix - good you responded before i sent out.
> >
> > 1/6 (act_vlan POP_ETH) has a similar unconditional
> > pskb_may_pull(ETH_HLEN). A brute approach is also to check for TEB:
> >
> >       if (skb->protocol == htons(ETH_P_TEB) &&  !pskb_may_pull(skb, ETH_HLEN))
> >                 goto drop;
> >
> > Note: skb_eth_pop() only removes the outer ethernet header but does
> > not change skb->protocol
> >
> 
> I am going to send it without this 1/6 change - it just doesn't feel
> like good taste. I will add a comment in the changes to say i
> considered it but decided not to. I also spent time eyeballing the
> code, i dont think there's a potential way to craft a packet that will
> hit the perceived issue. In any case the 24 hour embargo lifts in a
> few hours so i am not posting yet.
> 
> cheers,
> jamal
> 
> > thoughts?

I'm actually a little confused why the tc layer has to anticipate the
needs of downstream code and pull headers here.

That directly lead to this guessing of what may be the next header,
such as Ethernet. Which may often be correct, but not always.

All protocol parsing code itself should already be robust and call
pskb_may_pull where it is needed. Sounds like there are some targets
post TC where that is not true *and* we think there are too many to
update those targets directly, rather than this transformation logic
that exposes them to unpulled packets?