From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <stable-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
by inbox.dpdk.org (Postfix) with ESMTP id 62B99A0613
for <public@inbox.dpdk.org>; Tue, 30 Jul 2019 17:44:00 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
by dpdk.org (Postfix) with ESMTP id 27FAE1BF66;
Tue, 30 Jul 2019 17:44:00 +0200 (CEST)
Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28])
by dpdk.org (Postfix) with ESMTP id AEF831BF66;
Tue, 30 Jul 2019 17:43:58 +0200 (CEST)
Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com
[10.5.11.12])
(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))
(No client certificate requested)
by mx1.redhat.com (Postfix) with ESMTPS id B49A5C08E287;
Tue, 30 Jul 2019 15:43:57 +0000 (UTC)
Received: from dhcp-25.97.bos.redhat.com (unknown [10.18.25.67])
by smtp.corp.redhat.com (Postfix) with ESMTPS id EB91960C64;
Tue, 30 Jul 2019 15:43:52 +0000 (UTC)
From: Aaron Conole <aconole@redhat.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>
Cc: David Marchand <david.marchand@redhat.com>,
Bernard Iremonger <bernard.iremonger@intel.com>, dev <dev@dpdk.org>,
dpdk stable <stable@dpdk.org>, Thomas Monjalon <thomas@monjalon.net>, "Singh\,
Jasvinder" <jasvinder.singh@intel.com>,
Flavia Musatescu <flavia.musatescu@intel.com>,
Adrien Mazarguil <adrien.mazarguil@6wind.com>
References: <1562670596-27129-1-git-send-email-bernard.iremonger@intel.com>
<10372251.KTS5ePcUbj@xps>
<CAJFAV8zvor_rp_XORKGsQSjyifpvsW1ESfeX24cYb7_X5+F29Q@mail.gmail.com>
<f7tpnlrlh36.fsf@dhcp-25.97.bos.redhat.com>
<c87285dd-f322-809f-7cc3-3b41593da1be@intel.com>
Date: Tue, 30 Jul 2019 11:43:51 -0400
In-Reply-To: <c87285dd-f322-809f-7cc3-3b41593da1be@intel.com> (Ferruh Yigit's
message of "Tue, 30 Jul 2019 15:48:31 +0100")
Message-ID: <f7timrjle88.fsf@dhcp-25.97.bos.redhat.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12
X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16
(mx1.redhat.com [10.5.110.31]); Tue, 30 Jul 2019 15:43:58 +0000 (UTC)
Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] librte_flow_classify: fix
out-of-bounds access
X-BeenThere: stable@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: patches for DPDK stable branches <stable.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/stable>,
<mailto:stable-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/stable/>
List-Post: <mailto:stable@dpdk.org>
List-Help: <mailto:stable-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/stable>,
<mailto:stable-request@dpdk.org?subject=subscribe>
Errors-To: stable-bounces@dpdk.org
Sender: "stable" <stable-bounces@dpdk.org>
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