diff mbox series

[v2] gro: add missing invalid packet checks

Message ID 1547132768-2384-1-git-send-email-jiayu.hu@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v2] gro: add missing invalid packet checks |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

Hu, Jiayu Jan. 10, 2019, 3:06 p.m. UTC 
  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(+)

Comments

Thomas Monjalon Jan. 14, 2019, 10:26 p.m. UTC | #1 
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);
>
Stephen Hemminger Jan. 15, 2019, 1 a.m. UTC | #2 
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.
Hu, Jiayu Jan. 15, 2019, 2:48 a.m. UTC | #3 
> -----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.
Wang, Yinan Jan. 15, 2019, 5:05 a.m. UTC | #4 
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
Ananyev, Konstantin Jan. 15, 2019, 10:11 a.m. UTC | #5 
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
Hu, Jiayu Jan. 15, 2019, 12:18 p.m. UTC | #6 
> -----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
Hu, Jiayu Jan. 15, 2019, 1:38 p.m. UTC | #7 
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
diff mbox series

Patch

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