gro: fix overflow of TCP Options length calculation

  If we receive a packet with an invalid TCP header, whose
TCP header length is less than 20 bytes (the minimal TCP
header length), the calculated TCP Options length will
overflow and result in incorrect reassembly behaviors.

Fixes: 0d2cbe59b719 ("lib/gro: support TCP/IPv4")
Fixes: 9e0b9d2ec0f4 ("gro: support VxLAN GRO")
Cc: stable@dpdk.org

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 lib/librte_gro/gro_tcp4.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
  

On Fri, Jan 04, 2019 at 09:57:16AM +0800, Jiayu Hu wrote:
> If we receive a packet with an invalid TCP header, whose
> TCP header length is less than 20 bytes (the minimal TCP
> header length), the calculated TCP Options length will
> overflow and result in incorrect reassembly behaviors.

Please explain how changing the "len" type fixes this behaviour.

> 
> Fixes: 0d2cbe59b719 ("lib/gro: support TCP/IPv4")
> Fixes: 9e0b9d2ec0f4 ("gro: support VxLAN GRO")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
>  lib/librte_gro/gro_tcp4.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_gro/gro_tcp4.h b/lib/librte_gro/gro_tcp4.h
> index 6bb30cd..189cea3 100644
> --- a/lib/librte_gro/gro_tcp4.h
> +++ b/lib/librte_gro/gro_tcp4.h
> @@ -266,7 +266,8 @@ check_seq_option(struct gro_tcp4_item *item,
>  	struct rte_mbuf *pkt_orig = item->firstseg;
>  	struct ipv4_hdr *iph_orig;
>  	struct tcp_hdr *tcph_orig;
> -	uint16_t len, tcp_hl_orig;
> +	uint16_t tcp_hl_orig;
> +	int32_t len;
>  
>  	iph_orig = (struct ipv4_hdr *)(rte_pktmbuf_mtod(pkt_orig, char *) +
>  			l2_offset + pkt_orig->l2_len);
> -- 
> 2.7.4
>
  

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Monday, January 7, 2019 10:30 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>
> Cc: dev@dpdk.org; Bie, Tiwei <tiwei.bie@intel.com>; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] gro: fix overflow of TCP Options length
> calculation
> 
> On Fri, Jan 04, 2019 at 09:57:16AM +0800, Jiayu Hu wrote:
> > If we receive a packet with an invalid TCP header, whose
> > TCP header length is less than 20 bytes (the minimal TCP
> > header length), the calculated TCP Options length will
> > overflow and result in incorrect reassembly behaviors.
> 
> Please explain how changing the "len" type fixes this behaviour.

Originally, 'uint16_t len = RTE_MAX(tcp_hl, tcp_hl_orig) - sizeof(struct tcp_hdr)'.
When the TCP header length of an input packet is less than 20, which is the value of
sizeof(struct tcp_hdr), the value of len will overflow. For example, if TCP header lengths
of input packets are 14, the value of 'len' will be 65529 (65535-6). After then, we will
compare TCP options via memcmp(tcp_hdr+1,..., len), which would cause segment fault.

Changing the type of 'len' can just avoid segment fault. After the modification,
when applications input TCP packets which have illegal TCP headers, GRO just inserts
them into the table; it will not access TCP options and merge them. When users flush
the table or rte_gro_reassemble_burst() completes, all those invalid packets will be
evicted from the table.

In fact, if add minimal header lengths check in gro_vxlan_tcp4_reassemble() and
gro_tcp4_reassemble(), those packets with invalid headers will directly return to
applications; they will not go to check_seq_option() or insert into the table. Maybe
it's a better way for GRO to solve the issue from invalid packets. I will send a new
patch to solve the issue.

Thanks,
Jiayu
> 
> >
> > Fixes: 0d2cbe59b719 ("lib/gro: support TCP/IPv4")
> > Fixes: 9e0b9d2ec0f4 ("gro: support VxLAN GRO")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > ---
> >  lib/librte_gro/gro_tcp4.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_gro/gro_tcp4.h b/lib/librte_gro/gro_tcp4.h
> > index 6bb30cd..189cea3 100644
> > --- a/lib/librte_gro/gro_tcp4.h
> > +++ b/lib/librte_gro/gro_tcp4.h
> > @@ -266,7 +266,8 @@ check_seq_option(struct gro_tcp4_item *item,
> >  	struct rte_mbuf *pkt_orig = item->firstseg;
> >  	struct ipv4_hdr *iph_orig;
> >  	struct tcp_hdr *tcph_orig;
> > -	uint16_t len, tcp_hl_orig;
> > +	uint16_t tcp_hl_orig;
> > +	int32_t len;
> >
> >  	iph_orig = (struct ipv4_hdr *)(rte_pktmbuf_mtod(pkt_orig, char *) +
> >  			l2_offset + pkt_orig->l2_len);
> > --
> > 2.7.4
> >
  

On Tue, 8 Jan 2019 01:22:18 +0000
"Hu, Jiayu" <jiayu.hu@intel.com> wrote:

> > -----Original Message-----
> > From: Richardson, Bruce
> > Sent: Monday, January 7, 2019 10:30 PM
> > To: Hu, Jiayu <jiayu.hu@intel.com>
> > Cc: dev@dpdk.org; Bie, Tiwei <tiwei.bie@intel.com>; stable@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] gro: fix overflow of TCP Options length
> > calculation
> > 
> > On Fri, Jan 04, 2019 at 09:57:16AM +0800, Jiayu Hu wrote:  
> > > If we receive a packet with an invalid TCP header, whose
> > > TCP header length is less than 20 bytes (the minimal TCP
> > > header length), the calculated TCP Options length will
> > > overflow and result in incorrect reassembly behaviors.  
> > 
> > Please explain how changing the "len" type fixes this behaviour.  
> 
> Originally, 'uint16_t len = RTE_MAX(tcp_hl, tcp_hl_orig) - sizeof(struct tcp_hdr)'.
> When the TCP header length of an input packet is less than 20, which is the value of
> sizeof(struct tcp_hdr), the value of len will overflow. For example, if TCP header lengths
> of input packets are 14, the value of 'len' will be 65529 (65535-6). After then, we will
> compare TCP options via memcmp(tcp_hdr+1,..., len), which would cause segment fault.

For future safety, GRO should check header lengths for IP and TCP before looking
at packet. It is basic structure hygiene