[PATCH 0/3] app/testpmd: fix more overflow issues

[PATCH 0/3] app/testpmd: fix more overflow issues

[Stephen Hemminger]

While reviewing the flex item patches to test-pmd.
The AI review spotted some other problems.

Stephen Hemminger (3):
  app/testpmd: fix stack overflow parsing flex item link
  app/testpmd: fix flex item allocation overlap
  app/testpmd: fix null dereference parsing flex item link

 app/test-pmd/cmd_flex_item.c | 88 ++++++++++++++++++++----------------
 app/test-pmd/testpmd.h       |  7 +++
 2 files changed, 56 insertions(+), 39 deletions(-)

-- 
2.53.0

[PATCH 1/3] app/testpmd: fix stack overflow parsing flex item link

[Stephen Hemminger]

The item string in a flex item JSON configuration is formatted into a
256 byte stack buffer with sprintf().  A longer string overflows it:

  *** buffer overflow detected ***: terminated

Use snprintf() and reject the item if it does not fit.

Fixes: 59f3a8acbcdb ("app/testpmd: add flex item commands")
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test-pmd/cmd_flex_item.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/cmd_flex_item.c b/app/test-pmd/cmd_flex_item.c
index c0bbff7b45..e62afe3cb5 100644
--- a/app/test-pmd/cmd_flex_item.c
+++ b/app/test-pmd/cmd_flex_item.c
@@ -135,10 +135,13 @@ flex_link_item_parse(const char *src, struct rte_flow_item *item)
 	struct rte_flow_item *pattern;
 	struct rte_flow_action *actions;
 
-	sprintf(flow_rule,
-		"flow create 0 pattern %s / end actions drop / end", src);
-	src = flow_rule;
-	ret = flow_parse(src, (void *)data, sizeof(data),
+	ret = snprintf(flow_rule, sizeof(flow_rule),
+		       "flow create 0 pattern %s / end actions drop / end", src);
+	if (ret < 0 || ret >= (int)sizeof(flow_rule)) {
+		printf("Flex item link \"%s\" is too long\n", src);
+		return -ENOSPC;
+	}
+	ret = flow_parse(flow_rule, (void *)data, sizeof(data),
 			 &attr, &pattern, &actions);
 	if (ret)
 		return ret;
-- 
2.53.0

[PATCH 2/3] app/testpmd: fix flex item allocation overlap

[Stephen Hemminger]

flex_item_init() allocates struct flex_item together with the arrays it
points at, and places the first of them at base_size from the start of
the allocation.  base_size is the size of the embedded flex_conf rather
than the size of the whole struct, so sample_data starts inside struct
flex_item itself.  On x86-64 flex_conf is 112 bytes and struct flex_item
is 128, so sample_data[0] aliases flex_handle and flex_id.

Nothing reads flex_id and flex_handle is assigned after the config has
been consumed, so this is currently harmless, but any new member added
to the struct would be silently overwritten during parsing.

Fixes: 59f3a8acbcdb ("app/testpmd: add flex item commands")
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test-pmd/cmd_flex_item.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test-pmd/cmd_flex_item.c b/app/test-pmd/cmd_flex_item.c
index e62afe3cb5..f4560d2cec 100644
--- a/app/test-pmd/cmd_flex_item.c
+++ b/app/test-pmd/cmd_flex_item.c
@@ -305,7 +305,7 @@ flex_item_init(void)
 	uint8_t (*pattern)[FLEX_MAX_FLOW_PATTERN_LENGTH];
 	int i;
 
-	base_size = RTE_ALIGN(sizeof(*conf), sizeof(uintptr_t));
+	base_size = RTE_ALIGN(sizeof(*fp), sizeof(uintptr_t));
 	samples_size = RTE_ALIGN(FLEX_ITEM_MAX_SAMPLES_NUM *
 				 sizeof(conf->sample_data[0]),
 				 sizeof(uintptr_t));
-- 
2.53.0

[PATCH 3/3] app/testpmd: fix null dereference parsing flex item link

[Stephen Hemminger]

flex_item_init() hands the spec and mask buffers to the input links by
storing them in item.spec and item.mask, which are const pointers, and
never allocates anything for item.last.  A link item with a range then
copies into a null pointer:

  {"item": "eth type spec 2048 type last 2304 type mask 65535"}

The FLEX_LINK_IN check for item.last in flex_link_parse() runs after
flex_link_item_parse() has already copied, so it cannot prevent this.

Keep the buffers in the flex item as writable storage and pass them
down to the parser, which assigns them to the item only for what it
actually copied.  A range is rejected, and the copy is bounded by the
buffer size: the widest flow item mask is currently exactly
FLEX_MAX_FLOW_PATTERN_LENGTH bytes, so a new wider item would overflow
silently.

Fixes: 59f3a8acbcdb ("app/testpmd: add flex item commands")
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test-pmd/cmd_flex_item.c | 75 ++++++++++++++++++++----------------
 app/test-pmd/testpmd.h       |  7 ++++
 2 files changed, 48 insertions(+), 34 deletions(-)

diff --git a/app/test-pmd/cmd_flex_item.c b/app/test-pmd/cmd_flex_item.c
index f4560d2cec..3e55189a2d 100644
--- a/app/test-pmd/cmd_flex_item.c
+++ b/app/test-pmd/cmd_flex_item.c
@@ -124,12 +124,13 @@ enum flex_link_type {
 };
 
 static int
-flex_link_item_parse(const char *src, struct rte_flow_item *item)
+flex_link_item_parse(const char *src, struct rte_flow_item *item,
+		     struct flex_link_pattern *buf)
 {
 #define  FLEX_PARSE_DATA_SIZE 1024
 
 	int ret;
-	uint8_t *ptr, data[FLEX_PARSE_DATA_SIZE] = {0,};
+	uint8_t data[FLEX_PARSE_DATA_SIZE] = {0,};
 	char flow_rule[256];
 	struct rte_flow_attr *attr;
 	struct rte_flow_item *pattern;
@@ -146,31 +147,42 @@ flex_link_item_parse(const char *src, struct rte_flow_item *item)
 	if (ret)
 		return ret;
 	item->type = pattern->type;
+	item->spec = NULL;
+	item->mask = NULL;
+	item->last = NULL;
+	/* Only input links carry a value to match. */
+	if (buf == NULL)
+		return 0;
+	/* rte_flow_conv() reports the item size only if a mask is set. */
+	item->mask = buf->mask;
 	ret = rte_flow_conv(RTE_FLOW_CONV_OP_ITEM_MASK, NULL, 0, item, NULL);
-	if ((ret > 0) && pattern->spec) {
-		ptr = (void *)(uintptr_t)item->spec;
-		memcpy(ptr, pattern->spec, ret);
-	} else {
-		item->spec = NULL;
-	}
-	if ((ret > 0) && pattern->mask) {
-		ptr = (void *)(uintptr_t)item->mask;
-		memcpy(ptr, pattern->mask, ret);
-	} else {
+	if (ret <= 0) {
 		item->mask = NULL;
+		return 0;
 	}
-	if ((ret > 0) && pattern->last) {
-		ptr = (void *)(uintptr_t)item->last;
-		memcpy(ptr, pattern->last, ret);
-	} else {
-		item->last = NULL;
+	if (ret > FLEX_MAX_FLOW_PATTERN_LENGTH) {
+		printf("Flex item link \"%s\" needs %d bytes, maximum is %d\n",
+		       src, ret, FLEX_MAX_FLOW_PATTERN_LENGTH);
+		return -ENOSPC;
+	}
+	if (pattern->last != NULL) {
+		printf("Flex item link \"%s\" can not be a range\n", src);
+		return -ENOTSUP;
+	}
+	if (pattern->spec != NULL) {
+		memcpy(buf->spec, pattern->spec, ret);
+		item->spec = buf->spec;
 	}
+	if (pattern->mask != NULL)
+		memcpy(buf->mask, pattern->mask, ret);
+	else
+		item->mask = NULL;
 	return 0;
 }
 
 static int
 flex_link_parse(json_t *jobj, struct rte_flow_item_flex_link *link,
-		enum flex_link_type link_type)
+		struct flex_link_pattern *buf, enum flex_link_type link_type)
 {
 	const char *key;
 	json_t *je;
@@ -180,7 +192,7 @@ flex_link_parse(json_t *jobj, struct rte_flow_item_flex_link *link,
 			if (!json_is_string(je))
 				return -EINVAL;
 			ret = flex_link_item_parse(json_string_value(je),
-						   &link->item);
+						   &link->item, buf);
 			if (ret)
 				return -EINVAL;
 			if (link_type == FLEX_LINK_IN) {
@@ -204,9 +216,9 @@ flex_link_parse(json_t *jobj, struct rte_flow_item_flex_link *link,
 	return 0;
 }
 
-static int flex_item_config(json_t *jroot,
-			    struct rte_flow_item_flex_conf *flex_conf)
+static int flex_item_config(json_t *jroot, struct flex_item *fp)
 {
+	struct rte_flow_item_flex_conf *flex_conf = &fp->flex_conf;
 	const char *key;
 	json_t *jobj = NULL;
 	int ret = 0;
@@ -263,6 +275,7 @@ static int flex_item_config(json_t *jroot,
 				ji = json_array_get(jobj, i);
 				ret = flex_link_parse(ji,
 						      flex_conf->input_link + i,
+						      fp->link_pattern + i,
 						      FLEX_LINK_IN);
 				if (ret) {
 					printf("Can't parse input_link(s)\n");
@@ -283,7 +296,7 @@ static int flex_item_config(json_t *jroot,
 				ji = json_array_get(jobj, i);
 				ret = flex_link_parse
 					(ji, flex_conf->output_link + i,
-					 FLEX_LINK_OUT);
+					 NULL, FLEX_LINK_OUT);
 				if (ret) {
 					printf("Can't parse output_link(s)\n");
 					goto out;
@@ -299,11 +312,9 @@ static int flex_item_config(json_t *jroot,
 static struct flex_item *
 flex_item_init(void)
 {
-	size_t base_size, samples_size, links_size, spec_size;
+	size_t base_size, samples_size, links_size, pattern_size;
 	struct rte_flow_item_flex_conf *conf;
 	struct flex_item *fp;
-	uint8_t (*pattern)[FLEX_MAX_FLOW_PATTERN_LENGTH];
-	int i;
 
 	base_size = RTE_ALIGN(sizeof(*fp), sizeof(uintptr_t));
 	samples_size = RTE_ALIGN(FLEX_ITEM_MAX_SAMPLES_NUM *
@@ -313,8 +324,8 @@ flex_item_init(void)
 			       sizeof(conf->input_link[0]),
 			       sizeof(uintptr_t));
 	/* spec & mask for all input links */
-	spec_size = 2 * FLEX_MAX_FLOW_PATTERN_LENGTH * FLEX_ITEM_MAX_LINKS_NUM;
-	fp = calloc(1, base_size + samples_size + 2 * links_size + spec_size);
+	pattern_size = FLEX_ITEM_MAX_LINKS_NUM * sizeof(*fp->link_pattern);
+	fp = calloc(1, base_size + samples_size + 2 * links_size + pattern_size);
 	if (fp == NULL) {
 		printf("Can't allocate memory for flex item\n");
 		return NULL;
@@ -326,12 +337,8 @@ flex_item_init(void)
 			   ((uint8_t *)conf->sample_data + samples_size);
 	conf->output_link = (typeof(conf->output_link))
 			    ((uint8_t *)conf->input_link + links_size);
-	pattern = (typeof(pattern))((uint8_t *)conf->output_link + links_size);
-	for (i = 0; i < FLEX_ITEM_MAX_LINKS_NUM; i++) {
-		struct rte_flow_item_flex_link *in = conf->input_link + i;
-		in->item.spec = pattern++;
-		in->item.mask = pattern++;
-	}
+	fp->link_pattern = (typeof(fp->link_pattern))
+			   ((uint8_t *)conf->output_link + links_size);
 	return fp;
 }
 
@@ -346,7 +353,7 @@ flex_item_build_config(struct flex_item *fp, const char *filename)
 		printf("Bad JSON file \"%s\": %s\n", filename, json_error.text);
 		return -1;
 	}
-	ret = flex_item_config(jroot, &fp->flex_conf);
+	ret = flex_item_config(jroot, fp);
 	json_decref(jroot);
 	return ret;
 }
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index d23950ab9d..3a81464bab 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -445,8 +445,15 @@ void common_fwd_stream_init(struct fwd_stream *fs);
 #define FLEX_MAX_PATTERNS_NUM 64
 #define FLEX_PARSER_ERR ((struct flex_item *)-1)
 
+/** Spec and mask storage for one flex item input link. */
+struct flex_link_pattern {
+	uint8_t spec[FLEX_MAX_FLOW_PATTERN_LENGTH];
+	uint8_t mask[FLEX_MAX_FLOW_PATTERN_LENGTH];
+};
+
 struct flex_item {
 	struct rte_flow_item_flex_conf flex_conf;
+	struct flex_link_pattern *link_pattern;
 	struct rte_flow_item_flex_handle *flex_handle;
 	uint32_t flex_id;
 };
-- 
2.53.0