> -----Original Message-----
> From: Richardson, Bruce
> Sent: Monday, January 7, 2019 10:30 PM
> To: Hu, Jiayu <jiayu...@intel.com>
> Cc: dev@dpdk.org; Bie, Tiwei <tiwei....@intel.com>; sta...@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: sta...@dpdk.org
> >
> > Signed-off-by: Jiayu Hu <jiayu...@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
> >
