From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 1FA414CA0; Tue, 15 Jan 2019 13:18:07 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 15 Jan 2019 04:18:06 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,481,1539673200"; d="scan'208";a="126043873" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by FMSMGA003.fm.intel.com with ESMTP; 15 Jan 2019 04:18:07 -0800 Received: from fmsmsx120.amr.corp.intel.com (10.18.124.208) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.408.0; Tue, 15 Jan 2019 04:18:06 -0800 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx120.amr.corp.intel.com (10.18.124.208) with Microsoft SMTP Server (TLS) id 14.3.408.0; Tue, 15 Jan 2019 04:18:06 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.63]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.159]) with mapi id 14.03.0415.000; Tue, 15 Jan 2019 20:18:04 +0800 From: "Hu, Jiayu" To: "Ananyev, Konstantin" , "Wang, Yinan" , "dev@dpdk.org" CC: "thomas@monjalon.net" , "stable@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v2] gro: add missing invalid packet checks Thread-Index: AQHUqPYPT3KiVpCI1kS+zBXANWFesaWvR9QAgABVnICAAJyG4A== Date: Tue, 15 Jan 2019 12:18:03 +0000 Message-ID: References: <1546927725-68831-1-git-send-email-jiayu.hu@intel.com> <1547132768-2384-1-git-send-email-jiayu.hu@intel.com> <2601191342CEEE43887BDE71AB977258010D904112@irsmsx105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB977258010D904112@irsmsx105.ger.corp.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMDYxMjJmZTQtYTQyNy00NTQxLTg5MTUtMDI1NzZkMzc3ZWM4IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiT3hzXC9WVXZPeG5yVnVQejhTYmdMTTBJcFwvNEdHVDFHTFwvRWFcLzFkWmZSUE5meEt6OXdybFRNT0FrN08zbUdweU4ifQ== x-ctpclassification: CTP_NT x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="iso-2022-jp" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v2] gro: add missing invalid packet checks X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 15 Jan 2019 12:18:09 -0000 > -----Original Message----- > From: Ananyev, Konstantin > Sent: Tuesday, January 15, 2019 6:12 PM > To: Wang, Yinan ; Hu, Jiayu ; > dev@dpdk.org > Cc: thomas@monjalon.net; Hu, Jiayu ; > stable@dpdk.org > Subject: RE: [dpdk-dev] [PATCH v2] gro: add missing invalid packet checks >=20 > Hi, >=20 > > -----Original Message----- > > From: Wang, Yinan > > Sent: Tuesday, January 15, 2019 5:05 AM > > To: Hu, Jiayu ; dev@dpdk.org > > Cc: Ananyev, Konstantin ; > thomas@monjalon.net; Hu, Jiayu ; stable@dpdk.org > > Subject: RE: [dpdk-dev] [PATCH v2] gro: add missing invalid packet chec= ks > > > > Tested-by: Yinan Wang > > > > Best Wishes, > > Yinan > > > > > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jiayu Hu > > Sent: 2019=1B$BG/=1B(B1=1B$B7n=1B(B10=1B$BF|=1B(B 23:06 > > To: dev@dpdk.org > > Cc: Ananyev, Konstantin ; > thomas@monjalon.net; Hu, Jiayu ; stable@dpdk.org > > Subject: [dpdk-dev] [PATCH v2] gro: add missing invalid packet checks > > > > Currently, GRO library doesn't check if input packets have invalid head= ers. > The packets with invalid headers will also be processed by GRO. > > > > However, GRO shouldn't process invalid packets. This patch adds missing > invalid packet checks. > > > > Fixes: 0d2cbe59b719 ("lib/gro: support TCP/IPv4") > > Fixes: 9e0b9d2ec0f4 ("gro: support VxLAN GRO") > > Cc: stable@dpdk.org > > > > Signed-off-by: Jiayu Hu > > > > --- > > changes in v2: > > - fix VxLAN header length check bug for VxLAN GRO; > > - fix ethernet header length check bug; > > - use sizeof() and macro to present valid header length; > > - add VLAN related comments since GRO cannot process VLAN tagged > packets. > > > > lib/librte_gro/gro_tcp4.c | 12 ++++++++++++ > > lib/librte_gro/gro_tcp4.h | 10 ++++++++++ > > lib/librte_gro/gro_vxlan_tcp4.c | 15 +++++++++++++++ > > 3 files changed, 37 insertions(+) > > > > diff --git a/lib/librte_gro/gro_tcp4.c b/lib/librte_gro/gro_tcp4.c inde= x > 2fe9aab..48076e0 100644 > > --- a/lib/librte_gro/gro_tcp4.c > > +++ b/lib/librte_gro/gro_tcp4.c > > @@ -208,6 +208,18 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt, > > int cmp; > > uint8_t find; > > > > + /* > > + * Don't process the packet whose Ethernet, IPv4 and TCP header > > + * lengths are invalid. > > + * > > + * In addition, GRO doesn't process the packet that is VLAN > > + * tagged or whose the IPv4 header contains Options. > > + */ > > + if (unlikely(ILLEGAL_ETHER_HDRLEN(pkt->l2_len) || > > + ILLEGAL_IPV4_HDRLEN(pkt->l3_len) || > > + ILLEGAL_TCP_HDRLEN(pkt->l4_len))) > > + return -1; > > + > > eth_hdr =3D rte_pktmbuf_mtod(pkt, struct ether_hdr *); > > ipv4_hdr =3D (struct ipv4_hdr *)((char *)eth_hdr + pkt->l2_len); > > tcp_hdr =3D (struct tcp_hdr *)((char *)ipv4_hdr + pkt->l3_len); diff = --git > a/lib/librte_gro/gro_tcp4.h b/lib/librte_gro/gro_tcp4.h index > > 6bb30cd..65bcae8 100644 > > --- a/lib/librte_gro/gro_tcp4.h > > +++ b/lib/librte_gro/gro_tcp4.h > > @@ -17,6 +17,16 @@ > > */ > > #define MAX_IPV4_PKT_LENGTH UINT16_MAX > > > > +/* The maximum TCP header length */ > > +#define TCP_MAX_HLEN 60 > > + > > +#define ILLEGAL_ETHER_HDRLEN(len) ((len) !=3D ETHER_HDR_LEN) #define > > +ILLEGAL_ETHER_VXLAN_HDRLEN(len) \ > > + ((len) !=3D (ETHER_VXLAN_HLEN + ETHER_HDR_LEN)) #define > > +ILLEGAL_IPV4_HDRLEN(len) ((len) !=3D sizeof(struct ipv4_hdr)) #define > > +ILLEGAL_TCP_HDRLEN(len) \ > > + (((len) < sizeof(struct tcp_hdr)) || ((len) > TCP_MAX_HLEN)) > > + > > /* Header fields representing a TCP/IPv4 flow */ struct tcp4_flow_key= { > > struct ether_addr eth_saddr; > > diff --git a/lib/librte_gro/gro_vxlan_tcp4.c > b/lib/librte_gro/gro_vxlan_tcp4.c index 955ae4b..72d63bc 100644 > > --- a/lib/librte_gro/gro_vxlan_tcp4.c > > +++ b/lib/librte_gro/gro_vxlan_tcp4.c > > @@ -306,6 +306,21 @@ gro_vxlan_tcp4_reassemble(struct rte_mbuf > *pkt, > > uint16_t hdr_len; > > uint8_t find; > > > > + /* > > + * Don't process the packet whose outer Ethernet, outer IPv4, > > + * VxLAN header, inner Ethernet, inner IPv4 and inner TCP > > + * header lengths are invalid. > > + * > > + * In addition, GRO doesn't process the packet that is VLAN > > + * tagged or whose IPv4 header contains Options. > > + */ > > + if (unlikely(ILLEGAL_ETHER_HDRLEN(pkt->outer_l2_len) || > > + ILLEGAL_IPV4_HDRLEN(pkt->outer_l3_len) > || > > + ILLEGAL_ETHER_VXLAN_HDRLEN(pkt- > >l2_len) || > > + ILLEGAL_IPV4_HDRLEN(pkt->l3_len) || > > + ILLEGAL_TCP_HDRLEN(pkt->l4_len))) > > + return -1; > > + >=20 > Could you remind me why do we need that strict for l2 and l3 lenghts > (no ip options, no vlan, etc)? Reducing security risks is the motivation for those strict checks. If input packets have invalid header lengths, GRO cannot detect and will still process them. In fact, segment fault may happen in check_seq_option()= , if the input packet has TCP header length less than 20 bytes. It gives an opportunity to malicious attacks to make DPDK applications crash. Originally, I sent a patch to solve the segment fault issue (http://patches.dpdk.org/patch/49431/). But I think we need to a more thorough solution to handle the invalid packets. In the implementation of GRO, it doesn't consider VLAN tagged packets and packets with IPv4 Options. For example, we don't compare VLAN ID when merge packets. So I set the acceptable values of L2 and L3 to 14 and 20. > BTW, if we don't allow any other lenghts for l2/l3 except 14/20, > do we really need them to be setuped by user? The only reason is to reduce security risks (e.g. network attack). We use MBUF->packet_type to determine the input packet is performed GRO or not. If users input a packet with setting packet_type to (RTE_PTYPE_L4_TCP | RTE_PTYPE_L3_IPV4) but maliciously setting l2_len/l3_len to invalid values, GRO has no way to know it. IMO, thoroughly checking if input packets are malicious is not GRO's job. But if the overhead is low and adding some basic checks can avoid obvious errors, I think we can add those checks. Does it make sense? > Konstantin >=20 > > outer_eth_hdr =3D rte_pktmbuf_mtod(pkt, struct ether_hdr *); > > outer_ipv4_hdr =3D (struct ipv4_hdr *)((char *)outer_eth_hdr + > > pkt->outer_l2_len); > > -- > > 2.7.4