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 5829EA0613 for ; Tue, 30 Jul 2019 19:30:27 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 1D7EC1BFCF; Tue, 30 Jul 2019 19:30:27 +0200 (CEST) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id D846C1BFCE; Tue, 30 Jul 2019 19:30:24 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 37F9B3B71F; Tue, 30 Jul 2019 17:30:24 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (unknown [10.18.25.67]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8A9C95D6B2; Tue, 30 Jul 2019 17:30:20 +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 13:30:19 -0400 In-Reply-To: (Ferruh Yigit's message of "Tue, 30 Jul 2019 17:55:20 +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.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Tue, 30 Jul 2019 17:30:24 +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 4:43 PM, Aaron Conole wrote: >> 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; >> } > > +1 to unit test updates, lgtm. > > And I am for pushing the library updates to the next release, but please find a > few comments for now. Okay - I'll do that. But we probably will need to note that these APIs should get deprecated. Not sure if that should happen in this release - as the author of most of the code, maybe you would take care of that part? :) > >> >> 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; > > I think better to send '0' if the pointer is NULL, (instead of 1) Okay. Makes sense. > <...> > >> @@ -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); > > The doxygen comment is missing for 'pattern_l' & 'actions_l' but from code it is > number of items in the lists, this is duplication of the END marker information. > Instead, if those lengths are the length of the arrays will it be easier for the > user? So user won't need to calculate the item count but can pass the size of > the array. This still prevents API access out of the array. > > Anyway, as suggested above lets not make these decisions just a few days before > the release, but just get the unit test fix for the release, does it make sense? Sure. > And if so, can you send the unit test patch? Will do. > Thanks, > ferruh