[dpdk-dev] [PATCH v2] gro: add missing invalid packet checks

[Hu, Jiayu]

Hi Konstantin,

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Tuesday, January 15, 2019 6:12 PM
> To: Wang, Yinan <yinan.wang at intel.com>; Hu, Jiayu <jiayu.hu at intel.com>;
> dev at dpdk.org
> Cc: thomas at monjalon.net; Hu, Jiayu <jiayu.hu at intel.com>;
> stable at dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2] gro: add missing invalid packet checks
> 
> Hi,
> 
> > -----Original Message-----
> > From: Wang, Yinan
> > Sent: Tuesday, January 15, 2019 5:05 AM
> > To: Hu, Jiayu <jiayu.hu at intel.com>; dev at dpdk.org
> > Cc: Ananyev, Konstantin <konstantin.ananyev at intel.com>;
> thomas at monjalon.net; Hu, Jiayu <jiayu.hu at intel.com>; stable at dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v2] gro: add missing invalid packet checks
> >
> > Tested-by: Yinan Wang <yinan.wang at intel.com>
> >
> > Best Wishes,
> > Yinan
> >
> >
> > -----Original Message-----
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Jiayu Hu
> > Sent: 2019年1月10日 23:06
> > To: dev at dpdk.org
> > Cc: Ananyev, Konstantin <konstantin.ananyev at intel.com>;
> thomas at monjalon.net; Hu, Jiayu <jiayu.hu at intel.com>; stable at dpdk.org
> > Subject: [dpdk-dev] [PATCH v2] gro: add missing invalid packet checks
> >
> > Currently, GRO library doesn't check if input packets have invalid headers.
> 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 at dpdk.org
> >
> > Signed-off-by: Jiayu Hu <jiayu.hu at intel.com>
> >
> > ---
> > 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 index
> 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 = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
> >  	ipv4_hdr = (struct ipv4_hdr *)((char *)eth_hdr + pkt->l2_len);
> >  	tcp_hdr = (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) != ETHER_HDR_LEN) #define
> > +ILLEGAL_ETHER_VXLAN_HDRLEN(len) \
> > +	((len) != (ETHER_VXLAN_HLEN + ETHER_HDR_LEN)) #define
> > +ILLEGAL_IPV4_HDRLEN(len) ((len) != 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;
> > +
> 
> Could you remind me why do we need that strict for l2 and l3 lenghts
> (no ip options, no vlan, etc)?
> 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?

I think you are right. Checking if l2_len and l3_len are 14 or 20 makes
it meaningless to require users to set MBUF->l2_len/l3_len. The IPv4
and VLAN limitation should be well explained in the programmer guide.
We just need to check TCP header length to avoid segment fault in
check_seq_option().

I will rework this patch and add VLAN limitation in the another patch
http://patches.dpdk.org/patch/49505/ 

Really thanks for your suggestions.

Jiayu

> Konstantin
> 
> >  	outer_eth_hdr = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
> >  	outer_ipv4_hdr = (struct ipv4_hdr *)((char *)outer_eth_hdr +
> >  			pkt->outer_l2_len);
> > --
> > 2.7.4