• Florian Westphal's avatar
    net: mpls: error out if inner headers are not set · 025f8ad2
    Florian Westphal authored
    mpls_gso_segment() assumes skb_inner_network_header() returns
    a valid result:
    
      mpls_hlen = skb_inner_network_header(skb) - skb_network_header(skb);
      if (unlikely(!mpls_hlen || mpls_hlen % MPLS_HLEN))
            goto out;
      if (unlikely(!pskb_may_pull(skb, mpls_hlen)))
    
    With syzbot reproducer, skb_inner_network_header() yields 0,
    skb_network_header() returns 108, so this will
    "pskb_may_pull(skb, -108)))" which triggers a newly added
    DEBUG_NET_WARN_ON_ONCE() check:
    
    ------------[ 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
    [..]
     skb_mac_gso_segment+0x383/0x740 net/core/gso.c:53
     nsh_gso_segment+0x40a/0xad0 net/nsh/nsh.c:108
     skb_mac_gso_segment+0x383/0x740 net/core/gso.c:53
     __skb_gso_segment+0x324/0x4c0 net/core/gso.c:124
     skb_gso_segment include/net/gso.h:83 [inline]
     [..]
     sch_direct_xmit+0x11a/0x5f0 net/sched/sch_generic.c:327
     [..]
     packet_sendmsg+0x46a9/0x6130 net/packet/af_packet.c:3113
     [..]
    
    First iteration of this patch made mpls_hlen signed and changed
    test to error out to "mpls_hlen <= 0 || ..".
    
    Eric Dumazet said:
     > 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.
    
    So add new skb_inner_network_header_was_set() helper and use that.
    
    The syzbot reproducer injects data via packet socket. The skb that gets
    allocated and passed down the stack has ->protocol set to NSH (0x894f)
    and gso_type set to SKB_GSO_UDP | SKB_GSO_DODGY.
    
    This gets passed to skb_mac_gso_segment(), which sees NSH as ptype to
    find a callback for.  nsh_gso_segment() retrieves next type:
    
            proto = tun_p_to_eth_p(nsh_hdr(skb)->np);
    
    ... which is MPLS (TUN_P_MPLS_UC). It updates skb->protocol and then
    calls mpls_gso_segment().  Inner offsets are all 0, so mpls_gso_segment()
    ends up with a negative header size.
    
    In case more callers rely on silent handling of such large may_pull values
    we could also 'legalize' this behaviour, either replacing the debug check
    with (len > INT_MAX) test or removing it and instead adding a comment
    before existing
    
     if (unlikely(len > skb->len))
        return SKB_DROP_REASON_PKT_TOO_SMALL;
    
    test in pskb_may_pull_reason(), saying that this check also implicitly
    takes care of callers that miscompute header sizes.
    
    Cc: Simon Horman <horms@kernel.org>
    Fixes: 219eee9c ("net: skbuff: add overflow debug check to pull/push helpers")
    Reported-by: syzbot+99d15fcdb0132a1e1a82@syzkaller.appspotmail.com
    Closes: https://lore.kernel.org/netdev/00000000000043b1310611e388aa@google.com/rawSigned-off-by: default avatarFlorian Westphal <fw@strlen.de>
    Link: https://lore.kernel.org/r/20240222140321.14080-1-fw@strlen.deSigned-off-by: default avatarJakub Kicinski <kuba@kernel.org>
    025f8ad2
skbuff.h 144 KB