From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Hu, Jiayu" Subject: Re: [PATCH] gro: fix overflow of TCP Options length calculation Date: Tue, 8 Jan 2019 01:22:18 +0000 Message-ID: References: <1546567036-29444-1-git-send-email-jiayu.hu@intel.com> <20190107142955.GC14912@bricha3-MOBL.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" , "Bie, Tiwei" , "stable@dpdk.org" To: "Richardson, Bruce" Return-path: In-Reply-To: <20190107142955.GC14912@bricha3-MOBL.ger.corp.intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----Original Message----- > From: Richardson, Bruce > Sent: Monday, January 7, 2019 10:30 PM > To: Hu, Jiayu > Cc: dev@dpdk.org; Bie, Tiwei ; stable@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] gro: fix overflow of TCP Options length > calculation >=20 > 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. >=20 > Please explain how changing the "len" type fixes this behaviour. Originally, 'uint16_t len =3D 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 segme= nt fault. Changing the type of 'len' can just avoid segment fault. After the modifica= tion, when applications input TCP packets which have illegal TCP headers, GRO jus= t inserts them into the table; it will not access TCP options and merge them. When us= ers flush the table or rte_gro_reassemble_burst() completes, all those invalid packet= s 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 ret= urn to applications; they will not go to check_seq_option() or insert into the tab= le. Maybe it's a better way for GRO to solve the issue from invalid packets. I will s= end a new patch to solve the issue. Thanks, Jiayu >=20 > > > > Fixes: 0d2cbe59b719 ("lib/gro: support TCP/IPv4") > > Fixes: 9e0b9d2ec0f4 ("gro: support VxLAN GRO") > > Cc: stable@dpdk.org > > > > Signed-off-by: Jiayu Hu > > --- > > 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 =3D 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 =3D (struct ipv4_hdr *)(rte_pktmbuf_mtod(pkt_orig, char *) + > > l2_offset + pkt_orig->l2_len); > > -- > > 2.7.4 > >