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 E3598A0613 for ; Tue, 30 Jul 2019 18:55:27 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 926E81BF62; Tue, 30 Jul 2019 18:55:26 +0200 (CEST) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id AFF9F1BF1A; Tue, 30 Jul 2019 18:55:24 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 30 Jul 2019 09:55:23 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,327,1559545200"; d="scan'208";a="162634245" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.10]) ([10.237.221.10]) by orsmga007.jf.intel.com with ESMTP; 30 Jul 2019 09:55:21 -0700 To: Aaron Conole 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> From: Ferruh Yigit Openpgp: preference=signencrypt Autocrypt: addr=ferruh.yigit@intel.com; prefer-encrypt=mutual; keydata= mQINBFXZCFABEADCujshBOAaqPZpwShdkzkyGpJ15lmxiSr3jVMqOtQS/sB3FYLT0/d3+bvy qbL9YnlbPyRvZfnP3pXiKwkRoR1RJwEo2BOf6hxdzTmLRtGtwWzI9MwrUPj6n/ldiD58VAGQ +iR1I/z9UBUN/ZMksElA2D7Jgg7vZ78iKwNnd+vLBD6I61kVrZ45Vjo3r+pPOByUBXOUlxp9 GWEKKIrJ4eogqkVNSixN16VYK7xR+5OUkBYUO+sE6etSxCr7BahMPKxH+XPlZZjKrxciaWQb +dElz3Ab4Opl+ZT/bK2huX+W+NJBEBVzjTkhjSTjcyRdxvS1gwWRuXqAml/sh+KQjPV1PPHF YK5LcqLkle+OKTCa82OvUb7cr+ALxATIZXQkgmn+zFT8UzSS3aiBBohg3BtbTIWy51jNlYdy ezUZ4UxKSsFuUTPt+JjHQBvF7WKbmNGS3fCid5Iag4tWOfZoqiCNzxApkVugltxoc6rG2TyX CmI2rP0mQ0GOsGXA3+3c1MCdQFzdIn/5tLBZyKy4F54UFo35eOX8/g7OaE+xrgY/4bZjpxC1 1pd66AAtKb3aNXpHvIfkVV6NYloo52H+FUE5ZDPNCGD0/btFGPWmWRmkPybzColTy7fmPaGz cBcEEqHK4T0aY4UJmE7Ylvg255Kz7s6wGZe6IR3N0cKNv++O7QARAQABtCVGZXJydWggWWln aXQgPGZlcnJ1aC55aWdpdEBpbnRlbC5jb20+iQJUBBMBCgA+AhsDAh4BAheABQkI71rKFiEE 0jZTh0IuwoTjmYHH+TPrQ98TYR8FAlznMMQFCwkIBwMFFQoJCAsFFgIDAQAACgkQ+TPrQ98T YR/B9Q//a57esjq996nfZVm7AsUl7zbvhN+Ojity25ib2gcSVVsAN2j6lcQS4hf6/OVvRj3q CgebJ4o2gXR6X12UzWBJL7NE8Xpc70MvUIe0r11ykurQ9n9jUaWMjxdSqBPF93hU+Z/MZe5M 1rW5O2VJLuTJzkDw3EYUCbHOwPjeaS8Qqj3RI0LYbGthbHBIp9CsjkgsJSjTT5GQ8AQWkE7I z+hvPx6f1rllfjxFyi4DI3jLhAI+j1Nm+l+ESyoX59HrLTHAvq4RPkLpTnGBj9gOnJ+5sVEr GE0fcffsNcuMSkpqSEoJCPAHmChoLgezskhhsy0BiU3xlSIj1Dx2XMDerUXFOK3ftlbYNRte HQy4EKubfZRB8H5Rvcpksom3fRBDcJT8zw+PTH14htRApU9f8I/RamQ7Ujks7KuaB7JX5QaG gMjfPzHGYX9PfF6KIchaFmAWLytIP1t0ht8LpJkjtvUCSQZ2VxpCXwKyUzPDIF3co3tp90o7 X07uiC5ymX0K0+Owqs6zeslLY6DMxNdt8ye+h1TVkSZ5g4dCs4C/aiEF230+luL1CnejOv/K /s1iSbXQzJNM7be3FlRUz4FdwsfKiJJF7xYALSBnSvEB04R7I2P2V9Zpudkq6DRT6HZjBeJ1 pBF2J655cdoenPBIeimjnnh4K7YZBzwOLJf2c6u76fe5Ag0EV9ZMvgEQAKc0Db17xNqtSwEv mfp4tkddwW9XA0tWWKtY4KUdd/jijYqc3fDD54ESYpV8QWj0xK4YM0dLxnDU2IYxjEshSB1T qAatVWz9WtBYvzalsyTqMKP3w34FciuL7orXP4AibPtrHuIXWQOBECcVZTTOdZYGAzaYzxiA ONzF9eTiwIqe9/oaOjTwTLnOarHt16QApTYQSnxDUQljeNvKYt1lZE/gAUUxNLWsYyTT+22/ vU0GDUahsJxs1+f1yEr+OGrFiEAmqrzpF0lCS3f/3HVTU6rS9cK3glVUeaTF4+1SK5ZNO35p iVQCwphmxa+dwTG/DvvHYCtgOZorTJ+OHfvCnSVjsM4kcXGjJPy3JZmUtyL9UxEbYlrffGPQ I3gLXIGD5AN5XdAXFCjjaID/KR1c9RHd7Oaw0Pdcq9UtMLgM1vdX8RlDuMGPrj5sQrRVbgYH fVU/TQCk1C9KhzOwg4Ap2T3tE1umY/DqrXQgsgH71PXFucVjOyHMYXXugLT8YQ0gcBPHy9mZ qw5mgOI5lCl6d4uCcUT0l/OEtPG/rA1lxz8ctdFBVOQOxCvwRG2QCgcJ/UTn5vlivul+cThi 6ERPvjqjblLncQtRg8izj2qgmwQkvfj+h7Ex88bI8iWtu5+I3K3LmNz/UxHBSWEmUnkg4fJl Rr7oItHsZ0ia6wWQ8lQnABEBAAGJAjwEGAEKACYCGwwWIQTSNlOHQi7ChOOZgcf5M+tD3xNh HwUCXOcvZgUJBvIWKAAKCRD5M+tD3xNhHxhBD/9toXMIaPIVFd9w1nKsRDM1GE6gZe4jie8q MJpeHB9O+936fSXA0W2X0het60wJQQ45O8TpTcxpc9nGzcE4MTaLAI3E8TjIXAO0cPqUNLyp g0DXezmTw5BU+SKZ51+jSKOtFmzJCHOJZQaMeCHD+G3CrdUHQVQBb5AeuH3KFv9ltgDcWsc8 YO70o3+tGHwcEnyXLdrI0q05wV7ncnLdkgVo+VUN4092bNMPwYly1TZWcU3Jw5gczOUEfTY7 sgo6E/sGX3B+FzgIs5t4yi1XOweCAQ/mPnb6uFeNENEFyGKyMG1HtjwBqnftbiFO3qitEIUY xWGQH23oKscv7i9lT0gg2D+ktzZhVWwHJVY/2vWSB9aCSWChcH2BT+lWrkwSpoPhy+almM84 Qz2wF72/d4ce4L27pSrS+vOXtXHLGOOGcAn8yr9TV0kM4aR+NbGBRXGKhG6w4lY54uNd9IBa ARIPUhij5JSygxZCBaJKo+X64AHGkk5bXq+f0anwAMNuJXbYC/lz4DEdKmPgQGShOWNs1Y1a N3cI87Hun/RBVwQ0a3Tr1g6OWJ6xK8cYbMcoR8NZ7L9ALMeJeuUDQR39+fEeHg/6sQN0P0mv 0sL+//BAJphCzDk8ztbrFw+JaPtgzZpRSM6JhxnY+YMAsatJRXA0WSpYP5zzl7yu/GZJIgsv VQ== Message-ID: Date: Tue, 30 Jul 2019 17:55:20 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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" 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. > > 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) <...> > @@ -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? And if so, can you send the unit test patch? Thanks, ferruh