From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 6D14DA0613 for ; Tue, 30 Jul 2019 17:44:02 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 862521BFCB; 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 To: Ferruh Yigit Cc: David Marchand , Bernard Iremonger , dev , dpdk stable , Thomas Monjalon , "Singh\, Jasvinder" , Flavia Musatescu , Adrien Mazarguil References: <1562670596-27129-1-git-send-email-bernard.iremonger@intel.com> <10372251.KTS5ePcUbj@xps> Date: Tue, 30 Jul 2019 11:43:51 -0400 In-Reply-To: (Ferruh Yigit's message of "Tue, 30 Jul 2019 15:48:31 +0100") Message-ID: 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-dev] [dpdk-stable] [PATCH] librte_flow_classify: fix out-of-bounds access X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Ferruh Yigit writes: > On 7/30/2019 3:42 PM, Aaron Conole wrote: >> David Marchand writes: >> >>> On Wed, Jul 10, 2019 at 11:49 PM Thomas Monjalon 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 >>>> >>>> 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 --- 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