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

["dongchenchen (A)" <dongchenchen2@huawei.com>]

> 
> 
>>>>
>>> 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;
> +

Sorry, there is a typo here.
It should be SKB_DROP_REASON_SKB_BAD_GSO.

>                  if (!pskb_may_pull(skb, hdr_len + sizeof(struct tcphdr)))
>                          return SKB_DROP_REASON_SKB_BAD_GSO;
> 
> -------------
> Best Regards
> Dong Chenchen
> 
>> The issue specifically requires GSO packets (shinfo->gso_size != 0), 
>> which are
>> processed/aggregated through gro_cells. Fixing this in
>> gro_cells_receive() covers
>> all the relevant tunnel drivers in one place.
>>
>> Thanks.
>