Re: [PATCH net] net: iptunnel: fix stale transport header during tunnel decapsulation

quoted

quoted

quoted

Hi, Eric. Thanks for the review!

I missed the rcv path in the commit message.
With an ingress qdisc attached to gtp, the stale transport header
will been consumed in sch_handle_ingress() and its not covered by
gro_cells_receive() either. So maybe gtp is not safe by
construction.

gtp_encap_rcv
    gtp_rx //stale transport header
      __netif_rx
        __netif_receive_skb_core
          sch_handle_ingress
            qdisc_pkt_len_segs_init

Let's step back a bit :)

For non-GSO packets, qdisc_pkt_len_segs_init() returns immediately:

qdisc_skb_cb(skb)->pkt_len = skb->len;
if (!shinfo->gso_size) {
     qdisc_skb_cb(skb)->pkt_segs = 1;
     return SKB_NOT_DROPPED_YET;
}

It does not inspect skb->transport_header at all. It will be set
eventually later if needed,

GTP does not implement GRO on RX and does not use gro_cells, so incoming
packets in gtp_rx() have gso_size == 0.

Hi, Eric. Thanks for your patient explanation.

Normal GTP traffic indeed cannot trigger this issue and pkt without 
SKB_GSO_UDP_TUNNEL will be caught by udp_unexpected_gso.
The only way to reproduce this for gtp is by constructing and injecting 
packets, such as TAP device injects a GSO UDP-tunnel packet (with 
GSO_TCPV4 and GSO_UDP_TUNNEL flag, Ethernet/IPv4/UDP/GTP-U/IPV4/TCP).
Should we add interception for such illegal pkts?

diff --git a/net/core/dev.c b/net/core/dev.c
index af260ff5462a..8620428b950f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -4138,6 +4138,10 @@ static enum skb_drop_reason 

qdisc_pkt_len_segs_init(struct sk_buff *skb)
         if (likely(shinfo->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6))) {
                 const struct tcphdr *th;

+               /* Defensive: stale offsets from buggy tunnel decap can 
underflow. */
+               if (unlikely(hdr_len > INT_MAX))
+                       return 0;
+