From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 4E3DB1B3B8; Tue, 8 Jan 2019 09:14:14 +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 fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 08 Jan 2019 00:14:13 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,453,1539673200"; d="scan'208";a="124185832" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by FMSMGA003.fm.intel.com with ESMTP; 08 Jan 2019 00:14:12 -0800 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.408.0; Tue, 8 Jan 2019 00:14:12 -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, 8 Jan 2019 16:14:10 +0800 From: "Hu, Jiayu" To: Stephen Hemminger CC: "dev@dpdk.org" , "Bie, Tiwei" , "Richardson, Bruce" , "stable@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH] gro: add missing invalid packet checks Thread-Index: AQHUpxisSjPMmSc3QkGnTdYquJqwNaWkY1+AgACVKMA= Date: Tue, 8 Jan 2019 08:14:09 +0000 Message-ID: References: <1546567036-29444-1-git-send-email-jiayu.hu@intel.com> <1546927725-68831-1-git-send-email-jiayu.hu@intel.com> <20190107223151.18b185b7@hermes.lan> In-Reply-To: <20190107223151.18b185b7@hermes.lan> 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: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZDAxNWJmZjQtZWQ5Mi00MTkxLWI0NjUtNzFkZTIxYTM5MTAxIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiUHlxdDdSRnNQN0Z0Rk40VDNLQW5pS3N2c2ZtM041TU9iVVdVeHZySW9YQkUrOVVKUXVMZmRBcUE1bnBhcDdCeSJ9 x-ctpclassification: CTP_NT x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] 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, 08 Jan 2019 08:14:15 -0000 > -----Original Message----- > From: Stephen Hemminger [mailto:stephen@networkplumber.org] > Sent: Tuesday, January 8, 2019 2:32 PM > To: Hu, Jiayu > Cc: dev@dpdk.org; Bie, Tiwei ; Richardson, Bruce > ; stable@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] gro: add missing invalid packet checks >=20 > On Tue, 8 Jan 2019 14:08:45 +0800 > Jiayu Hu wrote: >=20 > > + /* > > + * Don't process the packet whose Ethernet, IPv4 and TCP header > > + * lengths are invalid. In addition, if the IPv4 header contains > > + * Options, the packet shouldn't be processed. > > + */ > > + if (unlikely(ILLEGAL_ETHER_HDRLEN(pkt->l2_len) || > > + ILLEGAL_IPV4_HDRLEN(pkt->l3_len) || > > + ILLEGAL_TCP_HDRLEN(pkt->l4_len))) > > + return -1; In the GRO design, we assume applications give correct MBUF->l2_len/.. for input packets of GRO. Specifically, GRO library assumes applications will set values to MBUF->l2_len/... and guarantee the values are the same as the values in the packet headers. The reason for this assumption is to process header faster. This is also why I want to add this assumption in the programmer guide. The above code is to forbid GRO to process invalid packets, which have invalid packet header lengths, like TCP header length is less than 20 bytes. >=20 > I like it when code is as picky as possible when doing optimizations beca= use > it reduces possible security riskg. >=20 > To me this looks more confusing and not as careful as doing it like: >=20 > if (unlikely(pkt->l2_len !=3D ETHER_HDR_LEN)) > return -1; > eth_hdr =3D rte_pktmbuf_mtod(pkt, struct ether_hdr *); > ipv4_hdr =3D (struct ipv4_hdr *)((char *)eth_hdr + ETHER_HDR_LEN); >=20 > if (pkt->l3_len !=3D (ipv4->version_ihl & IPV4_HDR_IHL_MASK) << 4) > return -1; >=20 > if (pkt->l4_len < sizeof(struct tcp_hdr)) > return -1; >=20 > You should also check for TCP options as well. There are two ways to get ether, ipv4 and tcp headers: 1). Use MBUF->l2_len/l3_len...; 2). Parse packet and ignore MBUF->l2_len/.... If we follow the choice 1, we don't need to parse packet and don't need to check if values of MBUF->l2_len/... are correct, since we assume applications will set correct values. If we follow the choice 2, we don't need to care about the values of MBUF->l2_len/... I am a little confused about your code, since it parses packet and checks if the values of MBUF->l2_len/... are correct. If we don't use MBUF->l2_len/... to get ether/ipv4/tcp headers, why should we check the values of MBUF->l2_len/...? Thanks, Jiayu >=20 > And IPv6 has same issues.