Skip to contentSkip to conversation

app/testpmd: fix more overflow issues

PATCH New Stephen Hemminger 1 participant 4 messages Sep 7

lore
Share & tools
lore

Stephen Hemminger

author
#0 lore raw

cover app/testpmd: fix more overflow issues

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(-)
Signature · 1 line
2.53.0
author
#1 lore raw

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

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 [email protected]
Signed-off-by Stephen Hemminger <[email protected]>
---
 app/test-pmd/cmd_flex_item.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
app/test-pmd/cmd_flex_item.c +7 -4
index c0bbff7b45..e62afe3cb5 100644
@@ -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;
Signature · 1 line
2.53.0
author
#2 lore raw

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

In reply to 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 [email protected]
Signed-off-by Stephen Hemminger <[email protected]>
---
 app/test-pmd/cmd_flex_item.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
app/test-pmd/cmd_flex_item.c +1 -1
index e62afe3cb5..f4560d2cec 100644
@@ -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));
Signature · 1 line
2.53.0
author
#3 lore raw

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

In reply to 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 [email protected]
Signed-off-by Stephen Hemminger <[email protected]>
---
 app/test-pmd/cmd_flex_item.c | 75 ++++++++++++++++++++----------------
 app/test-pmd/testpmd.h       |  7 ++++
 2 files changed, 48 insertions(+), 34 deletions(-)
app/test-pmd/cmd_flex_item.c +41 -34
index f4560d2cec..3e55189a2d 100644
@@ -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;
 }
app/test-pmd/testpmd.h +7 -0
index d23950ab9d..3a81464bab 100644
@@ -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;
 };
Signature · 1 line
2.53.0