WARNING in mpls_gso_segment

C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1536462c180000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/adbf5d8e38d7/disk-49344462.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/0f8e3fb78410/vmlinux-49344462.xz
kernel image: https://storage.googleapis.com/syzbot-assets/682f4814bf23/bzImage-49344462.xz

The issue was bisected to:

commit 219eee9c0d16f1b754a8b85275854ab17df0850a

Author: Florian Westphal <[email protected]>
Date:   Fri Feb 16 11:36:57 2024 +0000

net: skbuff: add overflow debug check to pull/push helpers

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=13262752180000
final oops:     https://syzkaller.appspot.com/x/report.txt?x=10a62752180000
console output: https://syzkaller.appspot.com/x/log.txt?x=17262752180000

IMPORTANT: if you fix the issue, please add the following tag to the commit:

Reported-by [email protected]

Fixes 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push helpers")

------------[ cut here ]------------
WARNING: CPU: 0 PID: 5068 at include/linux/skbuff.h:2723 pskb_may_pull_reason include/linux/skbuff.h:2723 [inline]
WARNING: CPU: 0 PID: 5068 at include/linux/skbuff.h:2723 pskb_may_pull include/linux/skbuff.h:2739 [inline]
WARNING: CPU: 0 PID: 5068 at include/linux/skbuff.h:2723 mpls_gso_segment+0x773/0xaa0 net/mpls/mpls_gso.c:34

Quoted 19 lines · syzbot

C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1536462c180000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/adbf5d8e38d7/disk-49344462.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/0f8e3fb78410/vmlinux-49344462.xz
kernel image: https://storage.googleapis.com/syzbot-assets/682f4814bf23/bzImage-49344462.xz

The issue was bisected to:

commit 219eee9c0d16f1b754a8b85275854ab17df0850a

Author: Florian Westphal <[email protected]>
Date:   Fri Feb 16 11:36:57 2024 +0000

net: skbuff: add overflow debug check to pull/push helpers

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=13262752180000
final oops:     https://syzkaller.appspot.com/x/report.txt?x=10a62752180000
console output: https://syzkaller.appspot.com/x/log.txt?x=17262752180000

Quoted 8 lines

IMPORTANT: if you fix the issue, please add the following tag to the commit:

Reported-by [email protected]

Fixes 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push helpers")

------------[ cut here ]------------
WARNING: CPU: 0 PID: 5068 at include/linux/skbuff.h:2723 pskb_may_pull_reason include/linux/skbuff.h:2723 [inline]
WARNING: CPU: 0 PID: 5068 at include/linux/skbuff.h:2723 pskb_may_pull include/linux/skbuff.h:2739 [inline]
WARNING: CPU: 0 PID: 5068 at include/linux/skbuff.h:2723 mpls_gso_segment+0x773/0xaa0 net/mpls/mpls_gso.c:34

Two possible solutions:

1.)

net/mpls/mpls_gso.c +0 -1

index 533d082f0701..43801b78dd64 100644
@@ -25,12 +25,13 @@ static struct sk_buff *mpls_gso_segment(struct sk_buff *skb,
        netdev_features_t mpls_features;
        u16 mac_len = skb->mac_len;
        __be16 mpls_protocol;
-       unsigned int mpls_hlen;

(or a variation thereof).

2) revert the pskb_may_pull_reason change added in 219eee9c0d16f1b754a8 to make it tolerant to "negative" (huge) may-pull requests again.

With above repro, skb_inner_network_header() yields 0, skb_network_header() returns 108, so we "pskb_may_pull(skb, -108)))" which now triggers DEBUG_NET_WARN_ON_ONCE() check.

Before blamed commit, this would make pskb_may_pull hit:

        if (unlikely(len > skb->len))
                return SKB_DROP_REASON_PKT_TOO_SMALL;

and mpls_gso_segment takes the 'goto out' label.

So question is really if we should fix this in mpls_gso (and possible others that try to pull negative numbers...) or if we should legalize this, either by adding explicit if (unlikely(len > INT_MAX)) test to pskb_may_pull_reason or by adding a comment that negative 'len' numbers are expected to be caught by the check vs. skb->len.

Opinions?

I guess we should try this, or perhaps understand why skb->encapsulation might not be set,
or why skb_inner_network_header(skb) is not set at this point.

Quoted 3 lines · Eric Dumazet

I guess we should try this, or perhaps understand why skb->encapsulation might not be set,
or why skb_inner_network_header(skb) is not set at this point.

syz repro injects data via packet socket, skb passed down stack has ->protocol set to NSH (0x894f), gso type is SKB_GSO_UDP | SKB_GSO_DODGY.

This gets passed down to skb_mac_gso_segment(), which sees NSH as ptype callback.

nsh_gso_segment() retrieves next type:

        proto = tun_p_to_eth_p(nsh_hdr(skb)->np);

... which is mpls (TUN_P_MPLS_UC), it then updates skb->protocol. This calls back into skb_mac_gso_segment() which sees MPLS as ptype callback, we then end up in mpls_gso_segment() without any inner headers set (skb->encapsulation is not set, inner header offsets are 0) and mpls_gso_segment() attempts to pull negative header size off the skb.

I don't see anything that could be done earlier in the stack about this.

As far as I understand NSH assumes its only called from openvswitch and MPLS GSO code only via Openvswitch or mpls_iptunnel, but its reachable by other means.

But skb_mac_gso_segment() doesn't have any info on the originator to know if it can call into nsh or mpls 'as intended'.

So I'd guess best solution is to explicitly check for negative header size, plus a comment that explains how this could happen.

I was thinking about adding a debug check in skb_inner_network_header(skb) if inner_network_header is zero (that would mean it is not 'set' yet), but this would trigger even after your patch.

Quoted 3 lines · Eric Dumazet

I was thinking about adding a debug check in skb_inner_network_header(skb) if inner_network_header is zero (that would mean it is not 'set' yet), but this would trigger even after your patch.

What about adding:

static inline bool skb_inner_network_header_was_set(const struct sk_buff *skb)

{
        return skb->inner_network_header > 0;
}

... and using that instead of checking for negative header length post-subtraction?

I was thinking about adding a debug check in skb_inner_network_header() if inner_network_header is zero (that would mean it is not 'set' yet), but this would trigger even after your patch.

Fixes 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push helpers")

Quoted 1 line · On Thu, 22 Feb 2024 15:03:10 +0100 Florian Westphal wrote:

Fixes 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push helpers")

This is for net-next, right?

When the network header pointer is greater than the inner network header, the difference between the two can cause mpls_hlen overflow.

Reported-and-tested-by [email protected]

Signed-off-by Lizhi Xu <[email protected]>

---
 net/mpls/mpls_gso.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

net/mpls/mpls_gso.c +2 -3

index 533d082f0701..2ab24b2fd90f 100644
@@ -25,11 +25,11 @@ static struct sk_buff *mpls_gso_segment(struct sk_buff *skb,
        netdev_features_t mpls_features;
        u16 mac_len = skb->mac_len;
        __be16 mpls_protocol;
-       unsigned int mpls_hlen;
+       int mpls_hlen;
 
        skb_reset_network_header(skb);
        mpls_hlen = skb_inner_network_header(skb) - skb_network_header(skb);
-       if (unlikely(!mpls_hlen || mpls_hlen % MPLS_HLEN))
+       if (unlikely(mpls_hlen <= 0 || mpls_hlen % MPLS_HLEN))
                goto out;
        if (unlikely(!pskb_may_pull(skb, mpls_hlen)))
                goto out;
--

2.43.0

Quoted 20 lines

When the network header pointer is greater than the inner network header, the difference between the two can cause mpls_hlen overflow.

Reported-and-tested-by [email protected]

Signed-off-by Lizhi Xu <[email protected]>

---
 net/mpls/mpls_gso.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

net/mpls/mpls_gso.c +1 -1

index 533d082f0701..2ab24b2fd90f 100644
@@ -25,11 +25,11 @@ static struct sk_buff *mpls_gso_segment(struct sk_buff *skb,
        netdev_features_t mpls_features;
        u16 mac_len = skb->mac_len;
        __be16 mpls_protocol;
-       unsigned int mpls_hlen;
+       int mpls_hlen;
 

Quoted 10 lines

net/mpls/mpls_gso.c +1 -2

        skb_reset_network_header(skb);
        mpls_hlen = skb_inner_network_header(skb) - skb_network_header(skb);
-       if (unlikely(!mpls_hlen || mpls_hlen % MPLS_HLEN))
+       if (unlikely(mpls_hlen <= 0 || mpls_hlen % MPLS_HLEN))
                goto out;
        if (unlikely(!pskb_may_pull(skb, mpls_hlen)))
                goto out;
--

2.43.0

I think Florian posted this patch, right ?

We must add a Fixes: tag

Also we should ask ourselves :
Why are we even looking at skb_inner_network_header(skb) if this was not set ?