[dpdk-dev] [dpdk-stable] [PATCH] librte_flow_classify: fix out-of-bounds access
Aaron Conole
aconole at redhat.com
Tue Jul 30 17:43:51 CEST 2019
Ferruh Yigit <ferruh.yigit at intel.com> writes:
> On 7/30/2019 3:42 PM, Aaron Conole wrote:
>> David Marchand <david.marchand at redhat.com> writes:
>>
>>> On Wed, Jul 10, 2019 at 11:49 PM Thomas Monjalon <thomas at 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 at dpdk.org
>>>>> Signed-off-by: Bernard Iremonger <bernard.iremonger at 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 at 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
More information about the dev
mailing list