Re: [dpdk-dev] [dpdk-stable] [PATCH] librte_flow_classify: fix out-of-bounds access

[Aaron Conole <aconole@redhat.com>]

Ferruh Yigit <ferruh.yigit@intel.com> writes:

> On 7/30/2019 3:42 PM, Aaron Conole wrote:
>> David Marchand <david.marchand@redhat.com> writes:
>> 
>>> On Wed, Jul 10, 2019 at 11:49 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>>>>
>>>> 09/07/2019 13:09, Bernard Iremonger:
>>>>> This patch fixes the out-of-bounds coverity issue by removing the
>>>>> offending line of code at line 107 in rte_flow_classify_parse.c
>>>>> which is never executed.
>>>>>
>>>>> Coverity issue: 343454
>>>>>
>>>>> Fixes: be41ac2a330f ("flow_classify: introduce flow classify library")
>>>>> Cc: stable@dpdk.org
>>>>> Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
>>>>
>>>> Applied, thanks
>>>
>>> We have a segfault in the unit tests since this patch.
>> 
>> I think this patch is still correct.  The issue is in the semantic of
>> the flow classify pattern.  It *MUST* always have a valid end marker,
>> but the test passes an invalid end marker.  This causes the bounds to
>> exceed.
>> 
>> So, it would be best to fix it, either by having a "failure" on unknown
>> markers (f.e. -1), or by passing a length around.  However, the crash
>> should be expected.  The fact that the previous code was also incorrect
>> and resulted in no segfault is pure luck.
>> 
>> See rte_flow_classify_parse.c:80 and test_flow_classify.c:387
>> 
>> I would be in favor of passing the lengths of the two arrays to these
>> APIs.  That would let us still make use of the markers (for valid
>> construction), but also let us reason about lengths in a sane way.
>> 
>> WDYT?
>> 
>
> +1, I also just replied with something very similar.
>
> With current API the testcase is wrong, and it will crash, also the invalid
> action one has exact same problem.
>
> The API can be updated as you suggested, with a length field and testcases can
> be added back.
>
> What worries me more is the rte_flow, which uses same arguments, and open to
> same errors, should we consider updating rte_flow APIs to have lengths values too?

Probably.

Here's a first crack at the change I think is appropriate.  I have done
some limited testing.  Let me know if you want me to submit it formally.

---------------------------- 8< ---------------------------------
Subject: [PATCH] rte_flow_classify: fix up the API and preserve ABI

Introduces a new API for doing length validations, and preserves the old semantics
and API.  The previous API couldn't handle corrupted end markers.  A future
version of the API might be able to eschew the end marker and trust the length,
but that is a discussion for future.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 app/test/test_flow_classify.c                | 30 +-------
 lib/librte_flow_classify/rte_flow_classify.c | 72 +++++++++++++++++---
 lib/librte_flow_classify/rte_flow_classify.h | 28 ++++++++
 3 files changed, 91 insertions(+), 39 deletions(-)

diff --git a/app/test/test_flow_classify.c b/app/test/test_flow_classify.c
index 6bbaad364..ff5265c6a 100644
--- a/app/test/test_flow_classify.c
+++ b/app/test/test_flow_classify.c
@@ -125,7 +125,6 @@ static struct rte_flow_item  udp_item_bad = { RTE_FLOW_ITEM_TYPE_UDP,
 
 static struct rte_flow_item  end_item = { RTE_FLOW_ITEM_TYPE_END,
 	0, 0, 0 };
-static struct rte_flow_item  end_item_bad = { -1, 0, 0, 0 };
 
 /* test TCP pattern:
  * "eth / ipv4 src spec 1.2.3.4 src mask 255.255.255.00 dst spec 5.6.7.8
@@ -181,7 +180,6 @@ static struct rte_flow_action count_action = { RTE_FLOW_ACTION_TYPE_COUNT,
 static struct rte_flow_action count_action_bad = { -1, 0};
 
 static struct rte_flow_action end_action = { RTE_FLOW_ACTION_TYPE_END, 0};
-static struct rte_flow_action end_action_bad =	{ -1, 0};
 
 static struct rte_flow_action actions[2];
 
@@ -384,7 +382,7 @@ test_invalid_patterns(void)
 
 	pattern[1] = ipv4_udp_item_1;
 	pattern[2] = udp_item_bad;
-	pattern[3] = end_item_bad;
+	pattern[3] = end_item;
 
 	ret = rte_flow_classify_validate(cls->cls, &attr, pattern,
 			actions, &error);
@@ -458,32 +456,6 @@ test_invalid_actions(void)
 		return -1;
 	}
 
-	actions[0] = count_action;
-	actions[1] = end_action_bad;
-
-	ret = rte_flow_classify_validate(cls->cls, &attr, pattern,
-			actions, &error);
-	if (!ret) {
-		printf("Line %i: rte_flow_classify_validate", __LINE__);
-		printf(" should have failed!\n");
-		return -1;
-	}
-
-	rule = rte_flow_classify_table_entry_add(cls->cls, &attr, pattern,
-			actions, &key_found, &error);
-	if (rule) {
-		printf("Line %i: flow_classify_table_entry_add", __LINE__);
-		printf(" should have failed!\n");
-		return -1;
-	}
-
-	ret = rte_flow_classify_table_entry_delete(cls->cls, rule);
-	if (!ret) {
-		printf("Line %i: rte_flow_classify_table_entry_delete",
-			__LINE__);
-		printf("should have failed!\n");
-		return -1;
-	}
 	return 0;
 }
 
diff --git a/lib/librte_flow_classify/rte_flow_classify.c b/lib/librte_flow_classify/rte_flow_classify.c
index 5ff585803..3ca1b1b44 100644
--- a/lib/librte_flow_classify/rte_flow_classify.c
+++ b/lib/librte_flow_classify/rte_flow_classify.c
@@ -89,18 +89,51 @@ struct rte_flow_classify_rule {
 	void *entry_ptr; /* handle to the table entry for rule meta data */
 };
 
+static size_t
+calc_flow_item_alen(const struct rte_flow_item pattern[])
+{
+	size_t i = 0;
+	while (pattern && (pattern + i)->type != RTE_FLOW_ITEM_TYPE_END)
+		i++;
+	return i + 1;
+}
+
+static size_t
+calc_flow_action_alen(const struct rte_flow_action actions[])
+{
+	size_t i = 0;
+	while (actions && (actions + i)->type != RTE_FLOW_ACTION_TYPE_END)
+		i++;
+	return i + 1;
+}
+
+int
+rte_flow_classify_validate(struct rte_flow_classifier *cls,
+			   const struct rte_flow_attr *attr,
+			   const struct rte_flow_item pattern[],
+			   const struct rte_flow_action actions[],
+			   struct rte_flow_error *error)
+{
+	return rte_flow_classify_validate_l(cls, attr, pattern,
+					    calc_flow_item_alen(pattern),
+					    actions,
+					    calc_flow_action_alen(actions),
+					    error);
+}
+
 int
-rte_flow_classify_validate(
-		   struct rte_flow_classifier *cls,
-		   const struct rte_flow_attr *attr,
-		   const struct rte_flow_item pattern[],
-		   const struct rte_flow_action actions[],
-		   struct rte_flow_error *error)
+rte_flow_classify_validate_l(struct rte_flow_classifier *cls,
+			     const struct rte_flow_attr *attr,
+			     const struct rte_flow_item pattern[],
+			     size_t pattern_l,
+			     const struct rte_flow_action actions[],
+			     size_t actions_l,
+			     struct rte_flow_error *error)
 {
 	struct rte_flow_item *items;
 	parse_filter_t parse_filter;
 	uint32_t item_num = 0;
-	uint32_t i = 0;
+	size_t i = 0;
 	int ret;
 
 	if (error == NULL)
@@ -134,17 +167,37 @@ rte_flow_classify_validate(
 		return -EINVAL;
 	}
 
+	while (i < actions_l && (actions + i)->type != RTE_FLOW_ACTION_TYPE_END)
+		i++;
+
+	if (i == actions_l) {
+		rte_flow_error_set(error, EINVAL,
+				   RTE_FLOW_ERROR_TYPE_ACTION_NUM,
+				   NULL, "Actions without end marker.");
+		return -EINVAL;
+	}
+
+	i = 0;
+
 	memset(&cls->ntuple_filter, 0, sizeof(cls->ntuple_filter));
 
 	/* Get the non-void item number of pattern */
-	while ((pattern + i)->type != RTE_FLOW_ITEM_TYPE_END) {
+	while (i < pattern_l && (pattern + i)->type != RTE_FLOW_ITEM_TYPE_END) {
 		if ((pattern + i)->type != RTE_FLOW_ITEM_TYPE_VOID)
 			item_num++;
 		i++;
 	}
+
 	item_num++;
 
-	items = malloc(item_num * sizeof(struct rte_flow_item));
+	if (i == pattern_l) {
+		rte_flow_error_set(error, EINVAL,
+				   RTE_FLOW_ERROR_TYPE_ITEM,
+				   NULL, "Pattern without end marker.");
+		return -EINVAL;
+	}
+
+	items = calloc(item_num, sizeof(struct rte_flow_item));
 	if (!items) {
 		rte_flow_error_set(error, ENOMEM,
 				RTE_FLOW_ERROR_TYPE_ITEM_NUM,
@@ -152,7 +205,6 @@ rte_flow_classify_validate(
 		return -ENOMEM;
 	}
 
-	memset(items, 0, item_num * sizeof(struct rte_flow_item));
 	classify_pattern_skip_void_item(items, pattern);
 
 	parse_filter = classify_find_parse_filter_func(items);
diff --git a/lib/librte_flow_classify/rte_flow_classify.h b/lib/librte_flow_classify/rte_flow_classify.h
index 74d1ecaf5..0308f6fd2 100644
--- a/lib/librte_flow_classify/rte_flow_classify.h
+++ b/lib/librte_flow_classify/rte_flow_classify.h
@@ -186,6 +186,34 @@ int
 rte_flow_classify_table_create(struct rte_flow_classifier *cls,
 		struct rte_flow_classify_table_params *params);
 
+/**
+ * Flow classify validate
+ *
+ * @param cls
+ *   Handle to flow classifier instance
+ * @param[in] attr
+ *   Flow rule attributes
+ * @param[in] pattern
+ *   Pattern specification (list terminated by the END pattern item).
+ * @param[in] actions
+ *   Associated actions (list terminated by the END pattern item).
+ * @param[out] error
+ *   Perform verbose error reporting if not NULL. Structure
+ *   initialised in case of error only.
+ * @return
+ *   0 on success, error code otherwise
+ */
+__rte_experimental
+int
+rte_flow_classify_validate_l(struct rte_flow_classifier *cls,
+			     const struct rte_flow_attr *attr,
+			     const struct rte_flow_item pattern[],
+			     const size_t pattern_l,
+			     const struct rte_flow_action actions[],
+			     const size_t actions_l,
+			     struct rte_flow_error *error);
+
+
 /**
  * Flow classify validate
  *
-- 
2.21.0