[PATCH 0/2] ip_frag: TAILQ and hash fixes

[PATCH 0/2] ip_frag: TAILQ and hash fixes

[Stephen Hemminger]

The ip_frag library had a couple of issues found during review.
The use of TAILQ made assumptions about TAILQ_REMOVE,
and the hash function usage could be abused by attacker.

Stephen Hemminger (2):
  ip_frag: fix unsafe TAILQ usage
  ip_frag: randomize hash seed

 lib/ip_frag/ip_frag_common.h     |  1 +
 lib/ip_frag/ip_frag_internal.c   | 18 ++++++++----------
 lib/ip_frag/ip_reassembly.h      |  1 +
 lib/ip_frag/rte_ip_frag_common.c | 21 +++++++++++----------
 4 files changed, 21 insertions(+), 20 deletions(-)

-- 
2.53.0

[PATCH 1/2] ip_frag: fix unsafe TAILQ usage

[Stephen Hemminger]

The frag table next pointer was being access after TAILQ_REMOVE().
This is not safe since it depends on TAILQ_REMOVE() not changing
next pointer. Fix by using RTE_TAILQ_FOREACH_SAFE().

Fixes: 95908f52393d ("ip_frag: free mbufs on reassembly table destroy")
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/ip_frag/ip_frag_common.h     |  1 +
 lib/ip_frag/rte_ip_frag_common.c | 20 ++++++++++----------
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/lib/ip_frag/ip_frag_common.h b/lib/ip_frag/ip_frag_common.h
index 51fc9d47fb..34be5bb6ab 100644
--- a/lib/ip_frag/ip_frag_common.h
+++ b/lib/ip_frag/ip_frag_common.h
@@ -8,6 +8,7 @@
 #include <sys/queue.h>
 
 #include <rte_common.h>
+#include <rte_tailq.h>
 
 #if defined(RTE_ARCH_ARM64)
 #include <rte_cmp_arm64.h>
diff --git a/lib/ip_frag/rte_ip_frag_common.c b/lib/ip_frag/rte_ip_frag_common.c
index ee9aa93027..79ac45289b 100644
--- a/lib/ip_frag/rte_ip_frag_common.c
+++ b/lib/ip_frag/rte_ip_frag_common.c
@@ -135,18 +135,18 @@ rte_ip_frag_table_del_expired_entries(struct rte_ip_frag_tbl *tbl,
 	struct rte_ip_frag_death_row *dr, uint64_t tms)
 {
 	uint64_t max_cycles;
-	struct ip_frag_pkt *fp;
+	struct ip_frag_pkt *fp, *tmp;
 
 	max_cycles = tbl->max_cycles;
 
-	TAILQ_FOREACH(fp, &tbl->lru, lru)
-		if (max_cycles + fp->start < tms) {
-			/* check that death row has enough space */
-			if (RTE_IP_FRAG_DEATH_ROW_MBUF_LEN - dr->cnt >=
-					fp->last_idx)
-				ip_frag_tbl_del(tbl, dr, fp);
-			else
-				return;
-		} else
+	RTE_TAILQ_FOREACH_SAFE(fp, &tbl->lru, lru, tmp) {
+		if (max_cycles + fp->start >= tms)
+			return;
+
+		/* check that death row has enough space */
+		if (RTE_IP_FRAG_DEATH_ROW_MBUF_LEN - dr->cnt < fp->last_idx)
 			return;
+
+		ip_frag_tbl_del(tbl, dr, fp);
+	}
 }
-- 
2.53.0

[PATCH 2/2] ip_frag: randomize hash seed

[Stephen Hemminger]

Both ipv4_frag_hash() and ipv6_frag_hash() use CRC32 (x86/ARM)
or jhash with a fixed, publicly known prime seed (0xeaad8405).
An attacker who can send crafted IP fragments can precompute hash
collisions, causing all fragments to land in the same bucket.
After bucket_entries concurrent flows collide, new flows are  dropped.

Fix by using a random hash seed added at table creation time.

Fixes: 416707812c03 ("ip_frag: refactor reassembly code into a proper library")
Cc: stable@dpdk.org
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/ip_frag/ip_frag_internal.c   | 18 ++++++++----------
 lib/ip_frag/ip_reassembly.h      |  1 +
 lib/ip_frag/rte_ip_frag_common.c |  1 +
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/lib/ip_frag/ip_frag_internal.c b/lib/ip_frag/ip_frag_internal.c
index 7cbef647df..382f42d0e1 100644
--- a/lib/ip_frag/ip_frag_internal.c
+++ b/lib/ip_frag/ip_frag_internal.c
@@ -9,8 +9,6 @@
 
 #include "ip_frag_common.h"
 
-#define	PRIME_VALUE	0xeaad8405
-
 #define	IP_FRAG_TBL_POS(tbl, sig)	\
 	((tbl)->pkt + ((sig) & (tbl)->entry_mask))
 
@@ -38,7 +36,7 @@ ip_frag_tbl_reuse(struct rte_ip_frag_tbl *tbl, struct rte_ip_frag_death_row *dr,
 
 
 static inline void
-ipv4_frag_hash(const struct ip_frag_key *key, uint32_t *v1, uint32_t *v2)
+ipv4_frag_hash(const struct ip_frag_key *key, uint32_t *v1, uint32_t *v2, uint32_t seed)
 {
 	uint32_t v;
 	const uint32_t *p;
@@ -46,12 +44,12 @@ ipv4_frag_hash(const struct ip_frag_key *key, uint32_t *v1, uint32_t *v2)
 	p = (const uint32_t *)&key->src_dst;
 
 #if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
-	v = rte_hash_crc_4byte(p[0], PRIME_VALUE);
+	v = rte_hash_crc_4byte(p[0], seed);
 	v = rte_hash_crc_4byte(p[1], v);
 	v = rte_hash_crc_4byte(key->id, v);
 #else
 
-	v = rte_jhash_3words(p[0], p[1], key->id, PRIME_VALUE);
+	v = rte_jhash_3words(p[0], p[1], key->id, seed);
 #endif /* RTE_ARCH_X86 */
 
 	*v1 =  v;
@@ -59,7 +57,7 @@ ipv4_frag_hash(const struct ip_frag_key *key, uint32_t *v1, uint32_t *v2)
 }
 
 static inline void
-ipv6_frag_hash(const struct ip_frag_key *key, uint32_t *v1, uint32_t *v2)
+ipv6_frag_hash(const struct ip_frag_key *key, uint32_t *v1, uint32_t *v2, uint32_t seed)
 {
 	uint32_t v;
 	const uint32_t *p;
@@ -67,7 +65,7 @@ ipv6_frag_hash(const struct ip_frag_key *key, uint32_t *v1, uint32_t *v2)
 	p = (const uint32_t *) &key->src_dst;
 
 #if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
-	v = rte_hash_crc_4byte(p[0], PRIME_VALUE);
+	v = rte_hash_crc_4byte(p[0], seed);
 	v = rte_hash_crc_4byte(p[1], v);
 	v = rte_hash_crc_4byte(p[2], v);
 	v = rte_hash_crc_4byte(p[3], v);
@@ -78,7 +76,7 @@ ipv6_frag_hash(const struct ip_frag_key *key, uint32_t *v1, uint32_t *v2)
 	v = rte_hash_crc_4byte(key->id, v);
 #else
 
-	v = rte_jhash_3words(p[0], p[1], p[2], PRIME_VALUE);
+	v = rte_jhash_3words(p[0], p[1], p[2], seed);
 	v = rte_jhash_3words(p[3], p[4], p[5], v);
 	v = rte_jhash_3words(p[6], p[7], key->id, v);
 #endif /* RTE_ARCH_X86 */
@@ -301,9 +299,9 @@ ip_frag_lookup(struct rte_ip_frag_tbl *tbl,
 
 	/* different hashing methods for IPv4 and IPv6 */
 	if (key->key_len == IPV4_KEYLEN)
-		ipv4_frag_hash(key, &sig1, &sig2);
+		ipv4_frag_hash(key, &sig1, &sig2, tbl->seed);
 	else
-		ipv6_frag_hash(key, &sig1, &sig2);
+		ipv6_frag_hash(key, &sig1, &sig2, tbl->seed);
 
 	p1 = IP_FRAG_TBL_POS(tbl, sig1);
 	p2 = IP_FRAG_TBL_POS(tbl, sig2);
diff --git a/lib/ip_frag/ip_reassembly.h b/lib/ip_frag/ip_reassembly.h
index 54afed5417..9e1666ef67 100644
--- a/lib/ip_frag/ip_reassembly.h
+++ b/lib/ip_frag/ip_reassembly.h
@@ -79,6 +79,7 @@ struct rte_ip_frag_tbl {
 	uint32_t bucket_entries; /* hash associativity. */
 	uint32_t nb_entries;     /* total size of the table. */
 	uint32_t nb_buckets;     /* num of associativity lines. */
+	uint32_t seed;		 /* hash function init value */
 	struct ip_frag_pkt *last;     /* last used entry. */
 	struct ip_pkt_list lru;       /* LRU list for table entries. */
 	struct ip_frag_tbl_stat stat; /* statistics counters. */
diff --git a/lib/ip_frag/rte_ip_frag_common.c b/lib/ip_frag/rte_ip_frag_common.c
index 79ac45289b..00bf9476de 100644
--- a/lib/ip_frag/rte_ip_frag_common.c
+++ b/lib/ip_frag/rte_ip_frag_common.c
@@ -79,6 +79,7 @@ rte_ip_frag_table_create(uint32_t bucket_num, uint32_t bucket_entries,
 	tbl->nb_buckets = bucket_num;
 	tbl->bucket_entries = bucket_entries;
 	tbl->entry_mask = (tbl->nb_entries - 1) & ~(tbl->bucket_entries  - 1);
+	tbl->seed = rte_rand();
 
 	TAILQ_INIT(&(tbl->lru));
 	return tbl;
-- 
2.53.0

RE: [PATCH 1/2] ip_frag: fix unsafe TAILQ usage

[Konstantin Ananyev]

> 
> The frag table next pointer was being access after TAILQ_REMOVE().
> This is not safe since it depends on TAILQ_REMOVE() not changing
> next pointer. Fix by using RTE_TAILQ_FOREACH_SAFE().
> 
> Fixes: 95908f52393d ("ip_frag: free mbufs on reassembly table destroy")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/ip_frag/ip_frag_common.h     |  1 +
>  lib/ip_frag/rte_ip_frag_common.c | 20 ++++++++++----------
>  2 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/ip_frag/ip_frag_common.h b/lib/ip_frag/ip_frag_common.h
> index 51fc9d47fb..34be5bb6ab 100644
> --- a/lib/ip_frag/ip_frag_common.h
> +++ b/lib/ip_frag/ip_frag_common.h
> @@ -8,6 +8,7 @@
>  #include <sys/queue.h>
> 
>  #include <rte_common.h>
> +#include <rte_tailq.h>
> 
>  #if defined(RTE_ARCH_ARM64)
>  #include <rte_cmp_arm64.h>
> diff --git a/lib/ip_frag/rte_ip_frag_common.c
> b/lib/ip_frag/rte_ip_frag_common.c
> index ee9aa93027..79ac45289b 100644
> --- a/lib/ip_frag/rte_ip_frag_common.c
> +++ b/lib/ip_frag/rte_ip_frag_common.c
> @@ -135,18 +135,18 @@ rte_ip_frag_table_del_expired_entries(struct
> rte_ip_frag_tbl *tbl,
>  	struct rte_ip_frag_death_row *dr, uint64_t tms)
>  {
>  	uint64_t max_cycles;
> -	struct ip_frag_pkt *fp;
> +	struct ip_frag_pkt *fp, *tmp;
> 
>  	max_cycles = tbl->max_cycles;
> 
> -	TAILQ_FOREACH(fp, &tbl->lru, lru)
> -		if (max_cycles + fp->start < tms) {
> -			/* check that death row has enough space */
> -			if (RTE_IP_FRAG_DEATH_ROW_MBUF_LEN - dr->cnt >=
> -					fp->last_idx)
> -				ip_frag_tbl_del(tbl, dr, fp);
> -			else
> -				return;
> -		} else
> +	RTE_TAILQ_FOREACH_SAFE(fp, &tbl->lru, lru, tmp) {
> +		if (max_cycles + fp->start >= tms)
> +			return;
> +
> +		/* check that death row has enough space */
> +		if (RTE_IP_FRAG_DEATH_ROW_MBUF_LEN - dr->cnt < fp-
> >last_idx)
>  			return;
> +
> +		ip_frag_tbl_del(tbl, dr, fp);
> +	}
>  }
> --

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

> 2.53.0

RE: [PATCH 2/2] ip_frag: randomize hash seed

[Konstantin Ananyev]

> Both ipv4_frag_hash() and ipv6_frag_hash() use CRC32 (x86/ARM)
> or jhash with a fixed, publicly known prime seed (0xeaad8405).
> An attacker who can send crafted IP fragments can precompute hash
> collisions, causing all fragments to land in the same bucket.
> After bucket_entries concurrent flows collide, new flows are  dropped.
> 
> Fix by using a random hash seed added at table creation time.
> 
> Fixes: 416707812c03 ("ip_frag: refactor reassembly code into a proper library")
> Cc: stable@dpdk.org
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/ip_frag/ip_frag_internal.c   | 18 ++++++++----------
>  lib/ip_frag/ip_reassembly.h      |  1 +
>  lib/ip_frag/rte_ip_frag_common.c |  1 +
>  3 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/ip_frag/ip_frag_internal.c b/lib/ip_frag/ip_frag_internal.c
> index 7cbef647df..382f42d0e1 100644
> --- a/lib/ip_frag/ip_frag_internal.c
> +++ b/lib/ip_frag/ip_frag_internal.c
> @@ -9,8 +9,6 @@
> 
>  #include "ip_frag_common.h"
> 
> -#define	PRIME_VALUE	0xeaad8405
> -
>  #define	IP_FRAG_TBL_POS(tbl, sig)	\
>  	((tbl)->pkt + ((sig) & (tbl)->entry_mask))
> 
> @@ -38,7 +36,7 @@ ip_frag_tbl_reuse(struct rte_ip_frag_tbl *tbl, struct
> rte_ip_frag_death_row *dr,
> 
> 
>  static inline void
> -ipv4_frag_hash(const struct ip_frag_key *key, uint32_t *v1, uint32_t *v2)
> +ipv4_frag_hash(const struct ip_frag_key *key, uint32_t *v1, uint32_t *v2,
> uint32_t seed)
>  {
>  	uint32_t v;
>  	const uint32_t *p;
> @@ -46,12 +44,12 @@ ipv4_frag_hash(const struct ip_frag_key *key, uint32_t
> *v1, uint32_t *v2)
>  	p = (const uint32_t *)&key->src_dst;
> 
>  #if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
> -	v = rte_hash_crc_4byte(p[0], PRIME_VALUE);
> +	v = rte_hash_crc_4byte(p[0], seed);
>  	v = rte_hash_crc_4byte(p[1], v);
>  	v = rte_hash_crc_4byte(key->id, v);
>  #else
> 
> -	v = rte_jhash_3words(p[0], p[1], key->id, PRIME_VALUE);
> +	v = rte_jhash_3words(p[0], p[1], key->id, seed);
>  #endif /* RTE_ARCH_X86 */
> 
>  	*v1 =  v;
> @@ -59,7 +57,7 @@ ipv4_frag_hash(const struct ip_frag_key *key, uint32_t *v1,
> uint32_t *v2)
>  }
> 
>  static inline void
> -ipv6_frag_hash(const struct ip_frag_key *key, uint32_t *v1, uint32_t *v2)
> +ipv6_frag_hash(const struct ip_frag_key *key, uint32_t *v1, uint32_t *v2,
> uint32_t seed)
>  {
>  	uint32_t v;
>  	const uint32_t *p;
> @@ -67,7 +65,7 @@ ipv6_frag_hash(const struct ip_frag_key *key, uint32_t *v1,
> uint32_t *v2)
>  	p = (const uint32_t *) &key->src_dst;
> 
>  #if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
> -	v = rte_hash_crc_4byte(p[0], PRIME_VALUE);
> +	v = rte_hash_crc_4byte(p[0], seed);
>  	v = rte_hash_crc_4byte(p[1], v);
>  	v = rte_hash_crc_4byte(p[2], v);
>  	v = rte_hash_crc_4byte(p[3], v);
> @@ -78,7 +76,7 @@ ipv6_frag_hash(const struct ip_frag_key *key, uint32_t *v1,
> uint32_t *v2)
>  	v = rte_hash_crc_4byte(key->id, v);
>  #else
> 
> -	v = rte_jhash_3words(p[0], p[1], p[2], PRIME_VALUE);
> +	v = rte_jhash_3words(p[0], p[1], p[2], seed);
>  	v = rte_jhash_3words(p[3], p[4], p[5], v);
>  	v = rte_jhash_3words(p[6], p[7], key->id, v);
>  #endif /* RTE_ARCH_X86 */
> @@ -301,9 +299,9 @@ ip_frag_lookup(struct rte_ip_frag_tbl *tbl,
> 
>  	/* different hashing methods for IPv4 and IPv6 */
>  	if (key->key_len == IPV4_KEYLEN)
> -		ipv4_frag_hash(key, &sig1, &sig2);
> +		ipv4_frag_hash(key, &sig1, &sig2, tbl->seed);
>  	else
> -		ipv6_frag_hash(key, &sig1, &sig2);
> +		ipv6_frag_hash(key, &sig1, &sig2, tbl->seed);
> 
>  	p1 = IP_FRAG_TBL_POS(tbl, sig1);
>  	p2 = IP_FRAG_TBL_POS(tbl, sig2);
> diff --git a/lib/ip_frag/ip_reassembly.h b/lib/ip_frag/ip_reassembly.h
> index 54afed5417..9e1666ef67 100644
> --- a/lib/ip_frag/ip_reassembly.h
> +++ b/lib/ip_frag/ip_reassembly.h
> @@ -79,6 +79,7 @@ struct rte_ip_frag_tbl {
>  	uint32_t bucket_entries; /* hash associativity. */
>  	uint32_t nb_entries;     /* total size of the table. */
>  	uint32_t nb_buckets;     /* num of associativity lines. */
> +	uint32_t seed;		 /* hash function init value */
>  	struct ip_frag_pkt *last;     /* last used entry. */
>  	struct ip_pkt_list lru;       /* LRU list for table entries. */
>  	struct ip_frag_tbl_stat stat; /* statistics counters. */
> diff --git a/lib/ip_frag/rte_ip_frag_common.c
> b/lib/ip_frag/rte_ip_frag_common.c
> index 79ac45289b..00bf9476de 100644
> --- a/lib/ip_frag/rte_ip_frag_common.c
> +++ b/lib/ip_frag/rte_ip_frag_common.c
> @@ -79,6 +79,7 @@ rte_ip_frag_table_create(uint32_t bucket_num, uint32_t
> bucket_entries,
>  	tbl->nb_buckets = bucket_num;
>  	tbl->bucket_entries = bucket_entries;
>  	tbl->entry_mask = (tbl->nb_entries - 1) & ~(tbl->bucket_entries  - 1);
> +	tbl->seed = rte_rand();
> 
>  	TAILQ_INIT(&(tbl->lru));
>  	return tbl;
> --

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

> 2.53.0