[dpdk-dev] [PATCH] gro: fix overflow of TCP Options length calculation

[dpdk-dev] [PATCH] gro: fix overflow of TCP Options length calculation

[Jiayu Hu]

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.

Fixes: 0d2cbe59b719 ("lib/gro: support TCP/IPv4")
Fixes: 9e0b9d2ec0f4 ("gro: support VxLAN GRO")
Cc: stable@dpdk.org

Signed-off-by: Jiayu Hu <jiayu.hu@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

Re: [dpdk-dev] [PATCH] gro: fix overflow of TCP Options length calculation

[Bruce Richardson]

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.

> 
> Fixes: 0d2cbe59b719 ("lib/gro: support TCP/IPv4")
> Fixes: 9e0b9d2ec0f4 ("gro: support VxLAN GRO")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@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
> 

Re: [dpdk-dev] [PATCH] gro: fix overflow of TCP Options length calculation

[Hu, Jiayu]

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

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

[Jiayu Hu]

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@dpdk.org

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 lib/librte_gro/gro_tcp4.c       | 10 ++++++++++
 lib/librte_gro/gro_tcp4.h       |  4 ++++
 lib/librte_gro/gro_vxlan_tcp4.c | 13 +++++++++++++
 3 files changed, 27 insertions(+)

diff --git a/lib/librte_gro/gro_tcp4.c b/lib/librte_gro/gro_tcp4.c
index 2fe9aab..0dc0de6 100644
--- a/lib/librte_gro/gro_tcp4.c
+++ b/lib/librte_gro/gro_tcp4.c
@@ -208,6 +208,16 @@ 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, 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;
+
 	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..cab84fd 100644
--- a/lib/librte_gro/gro_tcp4.h
+++ b/lib/librte_gro/gro_tcp4.h
@@ -17,6 +17,10 @@
  */
 #define MAX_IPV4_PKT_LENGTH UINT16_MAX
 
+#define ILLEGAL_ETHER_HDRLEN(len) ((len) < ETHER_HDR_LEN)
+#define ILLEGAL_IPV4_HDRLEN(len) ((len) != 20)
+#define ILLEGAL_TCP_HDRLEN(len) ((len) < 20 || (len) > 60)
+
 /* 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..c499c86 100644
--- a/lib/librte_gro/gro_vxlan_tcp4.c
+++ b/lib/librte_gro/gro_vxlan_tcp4.c
@@ -306,6 +306,19 @@ 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,
+	 * inner Ethernet, inner IPv4 and inner TCP header lengths
+	 * are invalid. In addition, if the outer or inner IPv4 header
+	 * contains Options, the packet shouldn't be processed.
+	 */
+	if (unlikely(ILLEGAL_ETHER_HDRLEN(pkt->outer_l2_len) ||
+				ILLEGAL_IPV4_HDRLEN(pkt->outer_l3_len) ||
+				ILLEGAL_ETHER_HDRLEN(pkt->l2_len) ||
+				ILLEGAL_IPV4_HDRLEN(pkt->l3_len) ||
+				ILLEGAL_TCP_HDRLEN(pkt->l4_len)))
+		return -1;
+
 	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

Re: [dpdk-dev] [PATCH] gro: fix overflow of TCP Options length calculation

[Stephen Hemminger]

On Tue, 8 Jan 2019 01:22:18 +0000
"Hu, Jiayu" <jiayu.hu@intel.com> wrote:

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

For future safety, GRO should check header lengths for IP and TCP before looking
at packet. It is basic structure hygiene

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

[Stephen Hemminger]

On Tue,  8 Jan 2019 14:08:45 +0800
Jiayu Hu <jiayu.hu@intel.com> wrote:

> +	/*
> +	 * 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;

I like it when code is as picky as possible when doing optimizations because
it reduces possible security riskg.

To me this looks more confusing and not as careful as doing it like:

	if (unlikely(pkt->l2_len != ETHER_HDR_LEN))
		return -1;
	eth_hdr = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
	ipv4_hdr = (struct ipv4_hdr *)((char *)eth_hdr + ETHER_HDR_LEN);

	if (pkt->l3_len != (ipv4->version_ihl & IPV4_HDR_IHL_MASK) << 4)
		return -1;

	if (pkt->l4_len < sizeof(struct tcp_hdr))
		return -1;

You should also check for TCP options as well.

And IPv6 has same issues.

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

[Hu, Jiayu]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, January 8, 2019 2:32 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>
> Cc: dev@dpdk.org; Bie, Tiwei <tiwei.bie@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] gro: add missing invalid packet checks
> 
> On Tue,  8 Jan 2019 14:08:45 +0800
> Jiayu Hu <jiayu.hu@intel.com> wrote:
> 
> > +	/*
> > +	 * 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.

> 
> I like it when code is as picky as possible when doing optimizations because
> it reduces possible security riskg.
> 
> To me this looks more confusing and not as careful as doing it like:
> 
> 	if (unlikely(pkt->l2_len != ETHER_HDR_LEN))
> 		return -1;
> 	eth_hdr = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
> 	ipv4_hdr = (struct ipv4_hdr *)((char *)eth_hdr + ETHER_HDR_LEN);
> 
> 	if (pkt->l3_len != (ipv4->version_ihl & IPV4_HDR_IHL_MASK) << 4)
> 		return -1;
> 
> 	if (pkt->l4_len < sizeof(struct tcp_hdr))
> 		return -1;
> 
> 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
> 
> And IPv6 has same issues.

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

[Ananyev, Konstantin]

> 
> 
> > -----Original Message-----
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Tuesday, January 8, 2019 2:32 PM
> > To: Hu, Jiayu <jiayu.hu@intel.com>
> > Cc: dev@dpdk.org; Bie, Tiwei <tiwei.bie@intel.com>; Richardson, Bruce
> > <bruce.richardson@intel.com>; stable@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] gro: add missing invalid packet checks
> >
> > On Tue,  8 Jan 2019 14:08:45 +0800
> > Jiayu Hu <jiayu.hu@intel.com> wrote:
> >
> > > +	/*
> > > +	 * 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.
> 
> >
> > I like it when code is as picky as possible when doing optimizations because
> > it reduces possible security riskg.
> >
> > To me this looks more confusing and not as careful as doing it like:
> >
> > 	if (unlikely(pkt->l2_len != ETHER_HDR_LEN))
> > 		return -1;
> > 	eth_hdr = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
> > 	ipv4_hdr = (struct ipv4_hdr *)((char *)eth_hdr + ETHER_HDR_LEN);
> >
> > 	if (pkt->l3_len != (ipv4->version_ihl & IPV4_HDR_IHL_MASK) << 4)
> > 		return -1;
> >
> > 	if (pkt->l4_len < sizeof(struct tcp_hdr))
> > 		return -1;
> >
> > 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/...?
> 

Agree that we don't need both.
My preference would be to stick with 1).
In many cases user would have already determined l2/l3/l4 len
by this stage.
Konstantin

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

[Morten Brørup]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> Konstantin
> Sent: Tuesday, January 8, 2019 11:39 AM
> To: Hu, Jiayu; Stephen Hemminger
> Cc: dev@dpdk.org; Bie, Tiwei; Richardson, Bruce; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] gro: add missing invalid packet checks
> 
> 
> 
> >
> >
> > > -----Original Message-----
> > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > Sent: Tuesday, January 8, 2019 2:32 PM
> > > To: Hu, Jiayu <jiayu.hu@intel.com>
> > > Cc: dev@dpdk.org; Bie, Tiwei <tiwei.bie@intel.com>; Richardson,
> Bruce
> > > <bruce.richardson@intel.com>; stable@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH] gro: add missing invalid packet
> checks
> > >
> > > On Tue,  8 Jan 2019 14:08:45 +0800
> > > Jiayu Hu <jiayu.hu@intel.com> wrote:
> > >
> > > > +	/*
> > > > +	 * 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.

+1 to more detailed documentation about assumptions and preconditions.


> >
> > 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.
> >
> > >
> > > I like it when code is as picky as possible when doing
> optimizations because
> > > it reduces possible security riskg.
> > >
> > > To me this looks more confusing and not as careful as doing it
> like:
> > >
> > > 	if (unlikely(pkt->l2_len != ETHER_HDR_LEN))
> > > 		return -1;
> > > 	eth_hdr = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
> > > 	ipv4_hdr = (struct ipv4_hdr *)((char *)eth_hdr + ETHER_HDR_LEN);
> > >
> > > 	if (pkt->l3_len != (ipv4->version_ihl & IPV4_HDR_IHL_MASK) << 4)
> > > 		return -1;
> > >
> > > 	if (pkt->l4_len < sizeof(struct tcp_hdr))
> > > 		return -1;
> > >
> > > 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/...?
> >
> 
> Agree that we don't need both.
> My preference would be to stick with 1).
> In many cases user would have already determined l2/l3/l4 len
> by this stage.
> Konstantin

Do we have a generic packet header validation library? Otherwise, that would perhaps be a better path. Such a library could probably use some of the flags from the PMD to determine how much to validate in software.

And if it is a documented precondition of the GRO library that m->l2_len/l3_len... must be set and sensible, perhaps an RTE_ASSERT() could be considered instead of gracefully returning -1?

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

[Hu, Jiayu]

> -----Original Message-----
> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Tuesday, January 8, 2019 7:34 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Hu, Jiayu
> <jiayu.hu@intel.com>; Stephen Hemminger
> <stephen@networkplumber.org>
> Cc: dev@dpdk.org; Bie, Tiwei <tiwei.bie@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] gro: add missing invalid packet checks
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> > Konstantin
> > Sent: Tuesday, January 8, 2019 11:39 AM
> > To: Hu, Jiayu; Stephen Hemminger
> > Cc: dev@dpdk.org; Bie, Tiwei; Richardson, Bruce; stable@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] gro: add missing invalid packet checks
> >
> >
> >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > > Sent: Tuesday, January 8, 2019 2:32 PM
> > > > To: Hu, Jiayu <jiayu.hu@intel.com>
> > > > Cc: dev@dpdk.org; Bie, Tiwei <tiwei.bie@intel.com>; Richardson,
> > Bruce
> > > > <bruce.richardson@intel.com>; stable@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH] gro: add missing invalid packet
> > checks
> > > >
> > > > On Tue,  8 Jan 2019 14:08:45 +0800
> > > > Jiayu Hu <jiayu.hu@intel.com> wrote:
> > > >
> > > > > +	/*
> > > > > +	 * 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.
> 
> +1 to more detailed documentation about assumptions and preconditions.
> 
> 
> > >
> > > 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.
> > >
> > > >
> > > > I like it when code is as picky as possible when doing
> > optimizations because
> > > > it reduces possible security riskg.
> > > >
> > > > To me this looks more confusing and not as careful as doing it
> > like:
> > > >
> > > > 	if (unlikely(pkt->l2_len != ETHER_HDR_LEN))
> > > > 		return -1;
> > > > 	eth_hdr = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
> > > > 	ipv4_hdr = (struct ipv4_hdr *)((char *)eth_hdr + ETHER_HDR_LEN);
> > > >
> > > > 	if (pkt->l3_len != (ipv4->version_ihl & IPV4_HDR_IHL_MASK) << 4)
> > > > 		return -1;
> > > >
> > > > 	if (pkt->l4_len < sizeof(struct tcp_hdr))
> > > > 		return -1;
> > > >
> > > > 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/...?
> > >
> >
> > Agree that we don't need both.
> > My preference would be to stick with 1).
> > In many cases user would have already determined l2/l3/l4 len
> > by this stage.
> > Konstantin
> 
> Do we have a generic packet header validation library? Otherwise, that
> would perhaps be a better path. Such a library could probably use some of
> the flags from the PMD to determine how much to validate in software.

As far as I know, we don't have the library to check if the values of MBUF->l2_len/...
are valid or not.

> 
> And if it is a documented precondition of the GRO library that m-
> >l2_len/l3_len... must be set and sensible, perhaps an RTE_ASSERT() could
> be considered instead of gracefully returning -1?

Comparing with terminating the application by RTE_ASSERT(), I think
not processing the invalid packet would be a better choice.

Thanks,
Jiayu

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

[Ananyev, Konstantin]

> -----Original Message-----
> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Tuesday, January 8, 2019 11:34 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Hu, Jiayu <jiayu.hu@intel.com>; Stephen Hemminger
> <stephen@networkplumber.org>
> Cc: dev@dpdk.org; Bie, Tiwei <tiwei.bie@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] gro: add missing invalid packet checks
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> > Konstantin
> > Sent: Tuesday, January 8, 2019 11:39 AM
> > To: Hu, Jiayu; Stephen Hemminger
> > Cc: dev@dpdk.org; Bie, Tiwei; Richardson, Bruce; stable@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] gro: add missing invalid packet checks
> >
> >
> >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > > Sent: Tuesday, January 8, 2019 2:32 PM
> > > > To: Hu, Jiayu <jiayu.hu@intel.com>
> > > > Cc: dev@dpdk.org; Bie, Tiwei <tiwei.bie@intel.com>; Richardson,
> > Bruce
> > > > <bruce.richardson@intel.com>; stable@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH] gro: add missing invalid packet
> > checks
> > > >
> > > > On Tue,  8 Jan 2019 14:08:45 +0800
> > > > Jiayu Hu <jiayu.hu@intel.com> wrote:
> > > >
> > > > > +	/*
> > > > > +	 * 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.
> 
> +1 to more detailed documentation about assumptions and preconditions.
> 
> 
> > >
> > > 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.
> > >
> > > >
> > > > I like it when code is as picky as possible when doing
> > optimizations because
> > > > it reduces possible security riskg.
> > > >
> > > > To me this looks more confusing and not as careful as doing it
> > like:
> > > >
> > > > 	if (unlikely(pkt->l2_len != ETHER_HDR_LEN))
> > > > 		return -1;
> > > > 	eth_hdr = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
> > > > 	ipv4_hdr = (struct ipv4_hdr *)((char *)eth_hdr + ETHER_HDR_LEN);
> > > >
> > > > 	if (pkt->l3_len != (ipv4->version_ihl & IPV4_HDR_IHL_MASK) << 4)
> > > > 		return -1;
> > > >
> > > > 	if (pkt->l4_len < sizeof(struct tcp_hdr))
> > > > 		return -1;
> > > >
> > > > 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/...?
> > >
> >
> > Agree that we don't need both.
> > My preference would be to stick with 1).
> > In many cases user would have already determined l2/l3/l4 len
> > by this stage.
> > Konstantin
> 
> Do we have a generic packet header validation library? Otherwise, that would perhaps be a better path. Such a library could probably use
> some of the flags from the PMD to determine how much to validate in software.

AFAIK - we don't have a generic header parsing library.
Yes, it would be good to have such ability, but I think that's out of scope for that patch.
BTW, volunteers are welcome :)

> 
> And if it is a documented precondition of the GRO library that m->l2_len/l3_len... must be set and sensible, perhaps an RTE_ASSERT() could
> be considered instead of gracefully returning -1?

I suppose that's too extreme.
What's wrong with checking input parameters and return an error if they are invalid?
Konstantin

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

[Morten Brørup]

> From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com]
> > From: Morten Brørup [mailto:mb@smartsharesystems.com]
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> > > Konstantin
> > > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > > >
> > > > > On Tue,  8 Jan 2019 14:08:45 +0800
> > > > > Jiayu Hu <jiayu.hu@intel.com> wrote:
> > > > >
> > > > > > +	/*
> > > > > > +	 * 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.
> >
> > +1 to more detailed documentation about assumptions and
> preconditions.
> >
> >
> > > >
> > > > 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.
> > > >
> > > > >
> > > > > I like it when code is as picky as possible when doing
> > > optimizations because
> > > > > it reduces possible security riskg.
> > > > >
> > > > > To me this looks more confusing and not as careful as doing it
> > > like:
> > > > >
> > > > > 	if (unlikely(pkt->l2_len != ETHER_HDR_LEN))
> > > > > 		return -1;
> > > > > 	eth_hdr = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
> > > > > 	ipv4_hdr = (struct ipv4_hdr *)((char *)eth_hdr +
> ETHER_HDR_LEN);
> > > > >
> > > > > 	if (pkt->l3_len != (ipv4->version_ihl & IPV4_HDR_IHL_MASK)
> << 4)
> > > > > 		return -1;
> > > > >
> > > > > 	if (pkt->l4_len < sizeof(struct tcp_hdr))
> > > > > 		return -1;
> > > > >
> > > > > 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/...?
> > > >
> > >
> > > Agree that we don't need both.
> > > My preference would be to stick with 1).
> > > In many cases user would have already determined l2/l3/l4 len
> > > by this stage.
> > > Konstantin
> >
> > Do we have a generic packet header validation library? Otherwise,
> that would perhaps be a better path. Such a library could probably use
> > some of the flags from the PMD to determine how much to validate in
> software.
> 
> AFAIK - we don't have a generic header parsing library.
> Yes, it would be good to have such ability, but I think that's out of
> scope for that patch.
> BTW, volunteers are welcome :)
> 
> >
> > And if it is a documented precondition of the GRO library that m-
> >l2_len/l3_len... must be set and sensible, perhaps an RTE_ASSERT()
> could
> > be considered instead of gracefully returning -1?
> 
> I suppose that's too extreme.
> What's wrong with checking input parameters and return an error if they
> are invalid?
> Konstantin
>
It is extreme, and it was partly meant as a provocation to think deeper about it: Do we really need to follow Postel's law (https://en.wikipedia.org/wiki/Robustness_principle) in functions where we are in full control ourselves?

Here's what's wrong with it: If each function a packet passes through has to parse the packet headers and validate the mbuf parameters from scratch, it will have an unnecessary performance cost. It should be done only once in the fast path, and subsequent functions should be able to rely on the result of that.

Generally, we should be able to rely on assumptions/preconditions. And when a function relies on something, it is a good thing to describe such preconditions in the function's documentation.

From a high level perspective, DPDK Core should not be a bunch of completely independent libraries, but a consistent library where its functions can rely on preconditions and postconditions of its other functions and their intended use in the fast path.

Regarding documentation, it might also be worth mentioning that the m->l2_len/... fields are described as "fields to support TX offloads" in rte_mbuf.h. So when RX offload features - such as GRO - rely on these fields, perhaps the description in rte_mbuf.h should be updated accordingly.


Med venlig hilsen / kind regards
- Morten Brørup

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

[Hu, Jiayu]

> -----Original Message-----
> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Tuesday, January 8, 2019 10:50 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Hu, Jiayu
> <jiayu.hu@intel.com>; Stephen Hemminger
> <stephen@networkplumber.org>
> Cc: dev@dpdk.org; Bie, Tiwei <tiwei.bie@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] gro: add missing invalid packet checks
> 
> > From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com]
> > > From: Morten Brørup [mailto:mb@smartsharesystems.com]
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> > > > Konstantin
> > > > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > > > >
> > > > > > On Tue,  8 Jan 2019 14:08:45 +0800
> > > > > > Jiayu Hu <jiayu.hu@intel.com> wrote:
> > > > > >
> > > > > > > +	/*
> > > > > > > +	 * 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.
> > >
> > > +1 to more detailed documentation about assumptions and
> > preconditions.
> > >
> > >
> > > > >
> > > > > 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.
> > > > >
> > > > > >
> > > > > > I like it when code is as picky as possible when doing
> > > > optimizations because
> > > > > > it reduces possible security riskg.
> > > > > >
> > > > > > To me this looks more confusing and not as careful as doing it
> > > > like:
> > > > > >
> > > > > > 	if (unlikely(pkt->l2_len != ETHER_HDR_LEN))
> > > > > > 		return -1;
> > > > > > 	eth_hdr = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
> > > > > > 	ipv4_hdr = (struct ipv4_hdr *)((char *)eth_hdr +
> > ETHER_HDR_LEN);
> > > > > >
> > > > > > 	if (pkt->l3_len != (ipv4->version_ihl & IPV4_HDR_IHL_MASK)
> > << 4)
> > > > > > 		return -1;
> > > > > >
> > > > > > 	if (pkt->l4_len < sizeof(struct tcp_hdr))
> > > > > > 		return -1;
> > > > > >
> > > > > > 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/...?
> > > > >
> > > >
> > > > Agree that we don't need both.
> > > > My preference would be to stick with 1).
> > > > In many cases user would have already determined l2/l3/l4 len
> > > > by this stage.
> > > > Konstantin
> > >
> > > Do we have a generic packet header validation library? Otherwise,
> > that would perhaps be a better path. Such a library could probably use
> > > some of the flags from the PMD to determine how much to validate in
> > software.
> >
> > AFAIK - we don't have a generic header parsing library.
> > Yes, it would be good to have such ability, but I think that's out of
> > scope for that patch.
> > BTW, volunteers are welcome :)
> >
> > >
> > > And if it is a documented precondition of the GRO library that m-
> > >l2_len/l3_len... must be set and sensible, perhaps an RTE_ASSERT()
> > could
> > > be considered instead of gracefully returning -1?
> >
> > I suppose that's too extreme.
> > What's wrong with checking input parameters and return an error if they
> > are invalid?
> > Konstantin
> >
> It is extreme, and it was partly meant as a provocation to think deeper
> about it: Do we really need to follow Postel's law
> (https://en.wikipedia.org/wiki/Robustness_principle) in functions where we
> are in full control ourselves?
> 
> Here's what's wrong with it: If each function a packet passes through has to
> parse the packet headers and validate the mbuf parameters from scratch, it
> will have an unnecessary performance cost. It should be done only once in
> the fast path, and subsequent functions should be able to rely on the result
> of that.
> 
> Generally, we should be able to rely on assumptions/preconditions. And
> when a function relies on something, it is a good thing to describe such
> preconditions in the function's documentation.
> 
> From a high level perspective, DPDK Core should not be a bunch of
> completely independent libraries, but a consistent library where its
> functions can rely on preconditions and postconditions of its other
> functions and their intended use in the fast path.

Good point.

> 
> Regarding documentation, it might also be worth mentioning that the m-
> >l2_len/... fields are described as "fields to support TX offloads" in
> rte_mbuf.h. So when RX offload features - such as GRO - rely on these fields,
> perhaps the description in rte_mbuf.h should be updated accordingly.

Yes, both IP reassembly and GRO use those fields. We need to update the description.

Thanks,
Jiayu
> 
> 
> Med venlig hilsen / kind regards
> - Morten Brørup

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

[Jiayu Hu]

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@dpdk.org

Signed-off-by: Jiayu Hu <jiayu.hu@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;
+
 	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

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

[Thomas Monjalon]

Any review, please?

10/01/2019 16:06, Jiayu Hu:
> 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@dpdk.org
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@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;
> +
>  	outer_eth_hdr = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
>  	outer_ipv4_hdr = (struct ipv4_hdr *)((char *)outer_eth_hdr +
>  			pkt->outer_l2_len);
> 

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

[Stephen Hemminger]

On Thu, 10 Jan 2019 23:06:08 +0800
Jiayu Hu <jiayu.hu@intel.com> wrote:

> +
> +#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))
> +

Why not inline (which keeps type checking) instead of macro.
Results in same code.

Also, prefer "invalid" instead "ILLEGAL" . There is no government inforcing
a rule on packet headers.

Also, what about ipv4 options, or TCP options?

And even VXLAN header check should be more rigorous.  What about not allowing
fragments in IP header for example.

If you are going to do enforcement, be as strict as you can.

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

[Hu, Jiayu]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, January 15, 2019 9:00 AM
> To: Hu, Jiayu <jiayu.hu@intel.com>
> Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> thomas@monjalon.net; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] gro: add missing invalid packet checks
> 
> On Thu, 10 Jan 2019 23:06:08 +0800
> Jiayu Hu <jiayu.hu@intel.com> wrote:
> 
> > +
> > +#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))
> > +
> 
> Why not inline (which keeps type checking) instead of macro.
> Results in same code.

You mean the inline functions like the following two?
static inline int check_invalid_hdr _tcp4(mbuf) {
	if (unlikely(mbuf->l2_len != ETHER_HDR_LEN ||
			mbuf->l3_len != sizeof(struct ipv4_hdr) ||
			mbuf->l4_len < sizeof(struct tcp_hdr) || mbuf->l4_len > 60))
		return 1;
	return 0;
}
static inline int check_invalid_hdr_vxlan_tcp4(mbuf) {
	if (unlikely(mbuf->outer_l2_len != ETHER_HDR_LEN ||
			mbuf->outer_l3_len != sizeof(struct ipv4_hdr) ||
			mbuf->l2_len != (ETHER_VXLAN_HLEN + ETHER_HDR_LEN) ||
			mbuf->l3_len != sizeof(struct ipv4_hdr) ||
			mbuf->l4_len < sizeof(struct tcp_hdr) || mbuf->l4_len > 60))
		return 1;
	return 0;
}
Or you mean every header length check should be one inline function, like:
check_invalid_ipv4_hdr(), check_invalid_vxlan_hdr() etc. ?

What is the main benefit of inline function for length check here?

> 
> Also, prefer "invalid" instead "ILLEGAL" . There is no government inforcing
> a rule on packet headers.

Thanks. I will change the name.

> 
> Also, what about ipv4 options, or TCP options?
Usually, IPv4 header doesn't have Options. Therefore, the
implementations of TCP and VxLAN GRO don't consider IPv4
Option fields when merge packets. For TCP Options, GRO checks
Option fields when merges packets. So in the above code, the
valid TCP header length is 20~60, and valid IPv4 header length is 20.

> 
> And even VXLAN header check should be more rigorous.  What about not
> allowing
> fragments in IP header for example.

Hmm, this is an assumption that all input packets are not IP fragments, which
is stated in programmer guide. But maybe too many assumptions make it hard
for users to use GRO. I will add this check.

BTW, if the received packets from PMD are IP fragments, their MBUF->packet_type
will include L4 protocols?

> 
> If you are going to do enforcement, be as strict as you can.

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

[Wang, Yinan]

Tested-by: Yinan Wang <yinan.wang@intel.com>

Best Wishes,
Yinan


-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jiayu Hu
Sent: 2019年1月10日 23:06
To: dev@dpdk.org
Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; thomas@monjalon.net; Hu, Jiayu <jiayu.hu@intel.com>; 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 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@dpdk.org

Signed-off-by: Jiayu Hu <jiayu.hu@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;
+
 	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

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

[Ananyev, Konstantin]

Hi,

> -----Original Message-----
> From: Wang, Yinan
> Sent: Tuesday, January 15, 2019 5:05 AM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; thomas@monjalon.net; Hu, Jiayu <jiayu.hu@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2] gro: add missing invalid packet checks
> 
> Tested-by: Yinan Wang <yinan.wang@intel.com>
> 
> Best Wishes,
> Yinan
> 
> 
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jiayu Hu
> Sent: 2019年1月10日 23:06
> To: dev@dpdk.org
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; thomas@monjalon.net; Hu, Jiayu <jiayu.hu@intel.com>; 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 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@dpdk.org
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@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?
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

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

[Hu, Jiayu]

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Tuesday, January 15, 2019 6:12 PM
> To: Wang, Yinan <yinan.wang@intel.com>; Hu, Jiayu <jiayu.hu@intel.com>;
> dev@dpdk.org
> Cc: thomas@monjalon.net; Hu, Jiayu <jiayu.hu@intel.com>;
> stable@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@intel.com>; dev@dpdk.org
> > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> thomas@monjalon.net; Hu, Jiayu <jiayu.hu@intel.com>; stable@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v2] gro: add missing invalid packet checks
> >
> > Tested-by: Yinan Wang <yinan.wang@intel.com>
> >
> > Best Wishes,
> > Yinan
> >
> >
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jiayu Hu
> > Sent: 2019年1月10日 23:06
> > To: dev@dpdk.org
> > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> thomas@monjalon.net; Hu, Jiayu <jiayu.hu@intel.com>; 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 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@dpdk.org
> >
> > Signed-off-by: Jiayu Hu <jiayu.hu@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)?

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
> 
> >  	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

Re: [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@intel.com>; Hu, Jiayu <jiayu.hu@intel.com>;
> dev@dpdk.org
> Cc: thomas@monjalon.net; Hu, Jiayu <jiayu.hu@intel.com>;
> stable@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@intel.com>; dev@dpdk.org
> > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> thomas@monjalon.net; Hu, Jiayu <jiayu.hu@intel.com>; stable@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v2] gro: add missing invalid packet checks
> >
> > Tested-by: Yinan Wang <yinan.wang@intel.com>
> >
> > Best Wishes,
> > Yinan
> >
> >
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jiayu Hu
> > Sent: 2019年1月10日 23:06
> > To: dev@dpdk.org
> > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> thomas@monjalon.net; Hu, Jiayu <jiayu.hu@intel.com>; 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 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@dpdk.org
> >
> > Signed-off-by: Jiayu Hu <jiayu.hu@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

[dpdk-dev] [PATCH v3] gro: add missing invalid TCP header length check

[Jiayu Hu]

When the TCP header length of input packets is invalid (i.e., less
than 20 bytes or greater than 60 bytes), check_seq_option() will
access illegal memory area when compare TCP Options, which may
cause a segmentation fault.

This patch adds missing invalid TCP header length check to avoid
illegal memory accesses.

Fixes: 0d2cbe59b719 ("lib/gro: support TCP/IPv4")
Fixes: 9e0b9d2ec0f4 ("gro: support VxLAN GRO")
Cc: stable@dpdk.org

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
Tested-by: Yinan Wang <yinan.wang@intel.com>
---
changes in v3:
- remove l2 and l3 length checks
- rename macro
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       | 7 +++++++
 lib/librte_gro/gro_tcp4.h       | 5 +++++
 lib/librte_gro/gro_vxlan_tcp4.c | 7 +++++++
 3 files changed, 19 insertions(+)

diff --git a/lib/librte_gro/gro_tcp4.c b/lib/librte_gro/gro_tcp4.c
index 2fe9aab..7d128a4 100644
--- a/lib/librte_gro/gro_tcp4.c
+++ b/lib/librte_gro/gro_tcp4.c
@@ -208,6 +208,13 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
 	int cmp;
 	uint8_t find;
 
+	/*
+	 * Don't process the packet whose TCP header length is greater
+	 * than 60 bytes or less than 20 bytes.
+	 */
+	if (unlikely(INVALID_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..d979248 100644
--- a/lib/librte_gro/gro_tcp4.h
+++ b/lib/librte_gro/gro_tcp4.h
@@ -17,6 +17,11 @@
  */
 #define MAX_IPV4_PKT_LENGTH UINT16_MAX
 
+/* The maximum TCP header length */
+#define MAX_TCP_HLEN 60
+#define INVALID_TCP_HDRLEN(len) \
+	(((len) < sizeof(struct tcp_hdr)) || ((len) > MAX_TCP_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..acb9bc9 100644
--- a/lib/librte_gro/gro_vxlan_tcp4.c
+++ b/lib/librte_gro/gro_vxlan_tcp4.c
@@ -306,6 +306,13 @@ gro_vxlan_tcp4_reassemble(struct rte_mbuf *pkt,
 	uint16_t hdr_len;
 	uint8_t find;
 
+	/*
+	 * Don't process the packet whose TCP header length is greater
+	 * than 60 bytes or less than 20 bytes.
+	 */
+	if (unlikely(INVALID_TCP_HDRLEN(pkt->l4_len)))
+		return -1;
+
 	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

Re: [dpdk-dev] [PATCH v3] gro: add missing invalid TCP header length check

[Ananyev, Konstantin]

> -----Original Message-----
> From: Hu, Jiayu
> Sent: Wednesday, January 16, 2019 12:46 AM
> To: dev@dpdk.org
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; thomas@monjalon.net; Hu, Jiayu <jiayu.hu@intel.com>; stable@dpdk.org
> Subject: [PATCH v3] gro: add missing invalid TCP header length check
> 
> When the TCP header length of input packets is invalid (i.e., less
> than 20 bytes or greater than 60 bytes), check_seq_option() will
> access illegal memory area when compare TCP Options, which may
> cause a segmentation fault.
> 
> This patch adds missing invalid TCP header length check to avoid
> illegal memory accesses.
> 
> Fixes: 0d2cbe59b719 ("lib/gro: support TCP/IPv4")
> Fixes: 9e0b9d2ec0f4 ("gro: support VxLAN GRO")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> Tested-by: Yinan Wang <yinan.wang@intel.com>
> ---

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> --
> 2.7.4

Re: [dpdk-dev] [PATCH v3] gro: add missing invalid TCP header length check

[Thomas Monjalon]

16/01/2019 10:49, Ananyev, Konstantin:
> From: Hu, Jiayu
> > 
> > When the TCP header length of input packets is invalid (i.e., less
> > than 20 bytes or greater than 60 bytes), check_seq_option() will
> > access illegal memory area when compare TCP Options, which may
> > cause a segmentation fault.
> > 
> > This patch adds missing invalid TCP header length check to avoid
> > illegal memory accesses.
> > 
> > Fixes: 0d2cbe59b719 ("lib/gro: support TCP/IPv4")
> > Fixes: 9e0b9d2ec0f4 ("gro: support VxLAN GRO")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > Tested-by: Yinan Wang <yinan.wang@intel.com>
> 
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Applied, thanks