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 B064CA0562; Fri, 3 Apr 2020 20:36:17 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 5370B1C1AD; Fri, 3 Apr 2020 20:36:17 +0200 (CEST) Received: from mailout1.w1.samsung.com (mailout1.w1.samsung.com [210.118.77.11]) by dpdk.org (Postfix) with ESMTP id 0FA321C190 for ; Fri, 3 Apr 2020 20:36:16 +0200 (CEST) Received: from eucas1p1.samsung.com (unknown [182.198.249.206]) by mailout1.w1.samsung.com (KnoxPortal) with ESMTP id 20200403183615euoutp01351b3baed1df3a0ad73191414a942c4d~CYxhh4cZ_2684626846euoutp01f for ; Fri, 3 Apr 2020 18:36:15 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout1.w1.samsung.com 20200403183615euoutp01351b3baed1df3a0ad73191414a942c4d~CYxhh4cZ_2684626846euoutp01f DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1585938975; bh=9eFkWyL0ffN2xek9/jLwkDMiiYIOXgoqDRGnqNmFY5E=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=iA3K//3VJwAhWI170MSr9+O7Cu9UDeCSzIKUeto7Qw06zGHRnnY3BogRBbr+q6m8x OWtg4V+xrc1EP3OqjIdrYY33lCifrt29WGX1SexcpottrhHVcwRUOzK8mdKfay8VQP 6kHOTUNL7qcyiJhIQnF3aveA5FFbH77x+8mepvas= Received: from eusmges3new.samsung.com (unknown [203.254.199.245]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20200403183614eucas1p299120c3b091308c8da6636ddd863dc3f~CYxhUFAPq2175821758eucas1p2H; Fri, 3 Apr 2020 18:36:14 +0000 (GMT) Received: from eucas1p2.samsung.com ( [182.198.249.207]) by eusmges3new.samsung.com (EUCPMTA) with SMTP id 61.77.60698.E12878E5; Fri, 3 Apr 2020 19:36:14 +0100 (BST) Received: from eusmtrp1.samsung.com (unknown [182.198.249.138]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20200403183614eucas1p12332c31aaf476b8893591f2e67c8f55c~CYxg-8jWj0302903029eucas1p1Y; Fri, 3 Apr 2020 18:36:14 +0000 (GMT) Received: from eusmgms2.samsung.com (unknown [182.198.249.180]) by eusmtrp1.samsung.com (KnoxPortal) with ESMTP id 20200403183614eusmtrp1ed07fcc0c5ae2ce120e0b277b8dbc5eb~CYxg-SF711717617176eusmtrp1X; Fri, 3 Apr 2020 18:36:14 +0000 (GMT) X-AuditID: cbfec7f5-a0fff7000001ed1a-b8-5e87821eefb8 Received: from eusmtip1.samsung.com ( [203.254.199.221]) by eusmgms2.samsung.com (EUCPMTA) with SMTP id 13.17.07950.E12878E5; Fri, 3 Apr 2020 19:36:14 +0100 (BST) Received: from [106.210.88.70] (unknown [106.210.88.70]) by eusmtip1.samsung.com (KnoxPortal) with ESMTPA id 20200403183613eusmtip11ed29dea4dc2a951f42342e498378c39~CYxgOBocK0734107341eusmtip1F; Fri, 3 Apr 2020 18:36:13 +0000 (GMT) To: Anoob Joseph , "dev@dpdk.org" Cc: Narayana Prasad Raju Athreya , "Lukas Bartosik [C]" From: Lukasz Wojciechowski Message-ID: Date: Fri, 3 Apr 2020 20:36:11 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 MIME-Version: 1.0 In-Reply-To: Content-Transfer-Encoding: 8bit Content-Language: pl X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFupjleLIzCtJLcpLzFFi42LZduzneV25pvY4g02HjCyWbdnKZPHu03Ym i5NH7rBY/HzZyu7A4vFrwVJWj8kLLzIHMEVx2aSk5mSWpRbp2yVwZSy+NJWx4JdzxeS1E1ga GJ9adDFyckgImEis6XrM2MXIxSEksIJR4tnedVDOF0aJ53t3sUM4nxklenv2s8O0rL/bygSR WM4osaG5jQ0kISTwFqh/Sy6ILSwQLnGr5ytYXETAReLcq0+sIDazQLrEvqbfYDabgK3EkZlf wWxeATeJF4/OMYHYLAIqEncOHAbrFRWIlTj36AZUjaDEyZlPWEBsTqD43r7p7BAz5SWat85m hrBFJG48agF7QUKgnV3ievtyFoirXSSuPtvKBGELS7w6vgXqGxmJ05N7WCAatjFKXP39E6p7 P6PE9d4VUFXWEof//QY6iQNohabE+l36EGFHibUtk1hAwhICfBI33gpCHMEnMWnbdGaIMK9E R5sQRLWexNOeqYwwa/+sfcIygVFpFpLXZiF5ZxaSd2Yh7F3AyLKKUTy1tDg3PbXYOC+1XK84 Mbe4NC9dLzk/dxMjMJWc/nf86w7GfX+SDjEKcDAq8fDeEGmPE2JNLCuuzD3EKMHBrCTC6zij NU6INyWxsiq1KD++qDQntfgQozQHi5I4r/Gil7FCAumJJanZqakFqUUwWSYOTqkGxuhVK0Qe dWwwDXrd9l+pttTmJEPV6nd7O+tWXTty6HPr4gMtz9KXvc/4qdJ86Jxfkc91JrOeHecu3nQ8 XZs9SfyJvt6Bl7t/3486OL2Pa91zu1SbzI2Lrjn+bvr2Y/XjfSnJik0af+LWLxWJWim6T1L6 /dHrndsiVraEdLjoKU3e8m9aR47ymn4lluKMREMt5qLiRAAVSZMeIQMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrAIsWRmVeSWpSXmKPExsVy+t/xu7pyTe1xBkdmKlgs27KVyeLdp+1M FieP3GGx+Pmyld2BxePXgqWsHpMXXmQOYIrSsynKLy1JVcjILy6xVYo2tDDSM7S00DMysdQz NDaPtTIyVdK3s0lJzcksSy3St0vQy1h8aSpjwS/nislrJ7A0MD616GLk5JAQMJFYf7eVqYuR i0NIYCmjxIWF/9m6GDmAEjISHy4JQNQIS/y51sUGUfOaUeL4nYPMIAlhgXCJWz1f2UBsEQEX iXOvPrGC2MwC6RIPXz9jhWhYxiTRtK0HrIhNwFbiyMyvYEW8Am4SLx6dYwKxWQRUJO4cOAxW IyoQK9HfvJsRokZQ4uTMJywgNidQfG/fdHaIBWYS8zY/ZIaw5SWat86GskUkbjxqYZzAKDQL SfssJC2zkLTMQtKygJFlFaNIamlxbnpusZFecWJucWleul5yfu4mRmDsbDv2c8sOxq53wYcY BTgYlXh4b4i0xwmxJpYVV+YeYpTgYFYS4XWc0RonxJuSWFmVWpQfX1Sak1p8iNEU6LmJzFKi yfnAuM4riTc0NTS3sDQ0NzY3NrNQEuftEDgYIySQnliSmp2aWpBaBNPHxMEp1cB4cO+WPJnN s8v8/kccu9U8dZ5JgSWbXq/88XMup+ev+2R+f8atoMdeJVM2epRq3flXVr/llvWbDvdDtl5B Cen781zk9zMqRC5guvvZczfHztDSWufQI26lX2z0ny/faJ2rtPKh4ObJq4+ufDWxdOGsRZti bkzybatsS1qYIfaMw+iekabnqugXSizFGYmGWsxFxYkAG/YtXrMCAAA= X-CMS-MailID: 20200403183614eucas1p12332c31aaf476b8893591f2e67c8f55c X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20200312151708eucas1p2acee543b5f9d236b8e43cd4d1fbed489 X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20200312151708eucas1p2acee543b5f9d236b8e43cd4d1fbed489 References: <20200312151654.7218-1-l.wojciechow@partner.samsung.com> <20200312151654.7218-2-l.wojciechow@partner.samsung.com> Subject: Re: [dpdk-dev] [PATCH 01/13] librte_security: fix verification of parameters 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" Hi Anoob, Thank you very much for your review. Please see my answers inline. Best regards, Lukasz W dniu 17.03.2020 o 13:59, Anoob Joseph pisze: > Hi Lukasz, > > Please see inline. > > Thanks, > Anoob > >> -----Original Message----- >> From: dev On Behalf Of Lukasz Wojciechowski >> Sent: Thursday, March 12, 2020 8:47 PM >> To: dev@dpdk.org >> Subject: [dpdk-dev] [PATCH 01/13] librte_security: fix verification of parameters > [Anoob] I believe the title has to be: "security: fix verification of parameters" > > Also, you can add "Fixes" as well. I changed the title and will push the new on in 2nd version of the paches after I'll fix all other issues. How do you add a "Fixes" tag to a patch? > >> This patch adds verification of the parameters to the ret_security API functions. >> All required parameters are checked if they are not NULL. >> >> Checks verify full chain of pointers, e.g. in case of verification of "instance->ops- >>> session_XXX", they check also "instance" and "instance->ops". >> Signed-off-by: Lukasz Wojciechowski >> Change-Id: I1724c926a1a0a13fd16d76f19842a0b40fbea1b2 >> --- >> lib/librte_security/rte_security.c | 58 +++++++++++++++++++++++------- >> 1 file changed, 45 insertions(+), 13 deletions(-) >> >> diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c >> index bc81ce15d..40a0e9ce5 100644 >> --- a/lib/librte_security/rte_security.c >> +++ b/lib/librte_security/rte_security.c >> @@ -1,6 +1,7 @@ >> /* SPDX-License-Identifier: BSD-3-Clause >> * Copyright 2017 NXP. >> * Copyright(c) 2017 Intel Corporation. >> + * Copyright (c) 2020 Samsung Electronics Co., Ltd All Rights Reserved >> */ >> >> #include >> @@ -9,6 +10,12 @@ >> #include "rte_security.h" >> #include "rte_security_driver.h" >> >> +/* Macro to check for invalid pointers */ >> +#define RTE_PTR_OR_ERR_RET(ptr, retval) do { \ >> + if ((ptr) == NULL) \ >> + return retval; \ >> +} while (0) >> + >> struct rte_security_session * >> rte_security_session_create(struct rte_security_ctx *instance, >> struct rte_security_session_conf *conf, @@ -16,10 >> +23,11 @@ rte_security_session_create(struct rte_security_ctx *instance, { >> struct rte_security_session *sess = NULL; >> >> - if (conf == NULL) >> - return NULL; >> - >> - RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_create, NULL); >> + RTE_PTR_OR_ERR_RET(instance, NULL); >> + RTE_PTR_OR_ERR_RET(instance->ops, NULL); >> + RTE_PTR_OR_ERR_RET(instance->ops->session_create, NULL); > [Anoob] The above three lines are repeated for every op NULL check. Can we introduce one macro for doing all the three checks? In case if it doesn't come off well, we can stick to individual checks. > Done. Will appear in 2nd version of patches. >> + RTE_PTR_OR_ERR_RET(conf, NULL); >> + RTE_PTR_OR_ERR_RET(mp, NULL); >> >> if (rte_mempool_get(mp, (void **)&sess)) >> return NULL; >> @@ -38,14 +46,20 @@ rte_security_session_update(struct rte_security_ctx >> *instance, >> struct rte_security_session *sess, >> struct rte_security_session_conf *conf) { >> - RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_update, - >> ENOTSUP); >> + RTE_PTR_OR_ERR_RET(instance, -EINVAL); >> + RTE_PTR_OR_ERR_RET(instance->ops, -EINVAL); >> + RTE_PTR_OR_ERR_RET(instance->ops->session_update, -ENOTSUP); >> + RTE_PTR_OR_ERR_RET(sess, -EINVAL); >> + RTE_PTR_OR_ERR_RET(conf, -EINVAL); >> return instance->ops->session_update(instance->device, sess, conf); } >> >> unsigned int >> rte_security_session_get_size(struct rte_security_ctx *instance) { >> - RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_get_size, 0); >> + RTE_PTR_OR_ERR_RET(instance, 0); >> + RTE_PTR_OR_ERR_RET(instance->ops, 0); >> + RTE_PTR_OR_ERR_RET(instance->ops->session_get_size, 0); >> return instance->ops->session_get_size(instance->device); >> } >> >> @@ -54,7 +68,11 @@ rte_security_session_stats_get(struct rte_security_ctx >> *instance, >> struct rte_security_session *sess, >> struct rte_security_stats *stats) { >> - RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_stats_get, - >> ENOTSUP); >> + RTE_PTR_OR_ERR_RET(instance, -EINVAL); >> + RTE_PTR_OR_ERR_RET(instance->ops, -EINVAL); >> + RTE_PTR_OR_ERR_RET(instance->ops->session_stats_get, -ENOTSUP); >> + // Parameter sess can be NULL in case of getting global statistics. > [Anoob] Checkpatch error. > ERROR:C99_COMMENTS: do not use C99 // comments Done. Will appear in 2nd version of patches. > >> + RTE_PTR_OR_ERR_RET(stats, -EINVAL); >> return instance->ops->session_stats_get(instance->device, sess, stats); >> } >> >> @@ -64,7 +82,10 @@ rte_security_session_destroy(struct rte_security_ctx >> *instance, { >> int ret; >> >> - RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_destroy, - >> ENOTSUP); >> + RTE_PTR_OR_ERR_RET(instance, -EINVAL); >> + RTE_PTR_OR_ERR_RET(instance->ops, -EINVAL); >> + RTE_PTR_OR_ERR_RET(instance->ops->session_destroy, -ENOTSUP); >> + RTE_PTR_OR_ERR_RET(sess, -EINVAL); >> >> if (instance->sess_cnt) >> instance->sess_cnt--; >> @@ -81,7 +102,11 @@ rte_security_set_pkt_metadata(struct rte_security_ctx >> *instance, >> struct rte_security_session *sess, >> struct rte_mbuf *m, void *params) { >> - RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->set_pkt_metadata, - >> ENOTSUP); >> + RTE_PTR_OR_ERR_RET(instance, -EINVAL); >> + RTE_PTR_OR_ERR_RET(instance->ops, -EINVAL); >> + RTE_PTR_OR_ERR_RET(instance->ops->set_pkt_metadata, -ENOTSUP); > [Anoob] set_pkt_metadata() and get_userdata() are datapath ops. So can you introduce a config option to enable/disable the checks. > > Please check, > https://protect2.fireeye.com/url?k=c52d8c32-98e14097-c52c077d-0cc47a30d446-c1b9d873e3e59cc4&u=http://code.dpdk.org/dpdk/latest/source/lib/librte_ethdev/rte_ethdev.h#L4372 Could you explain a bit further? Do you propose to make checks inside #ifdef RTE_LIBRTE_SECURITY_DEBUG or so? And do you have all checks or just sess and m on mind? The instance->ops->function checks were already there without any config options in all API functions. > >> + RTE_PTR_OR_ERR_RET(sess, -EINVAL); >> + RTE_PTR_OR_ERR_RET(m, -EINVAL); >> return instance->ops->set_pkt_metadata(instance->device, >> sess, m, params); >> } >> @@ -91,7 +116,9 @@ rte_security_get_userdata(struct rte_security_ctx >> *instance, uint64_t md) { >> void *userdata = NULL; >> >> - RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->get_userdata, NULL); >> + RTE_PTR_OR_ERR_RET(instance, NULL); >> + RTE_PTR_OR_ERR_RET(instance->ops, NULL); >> + RTE_PTR_OR_ERR_RET(instance->ops->get_userdata, NULL); >> if (instance->ops->get_userdata(instance->device, md, &userdata)) >> return NULL; >> >> @@ -101,7 +128,9 @@ rte_security_get_userdata(struct rte_security_ctx >> *instance, uint64_t md) const struct rte_security_capability * >> rte_security_capabilities_get(struct rte_security_ctx *instance) { >> - RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->capabilities_get, NULL); >> + RTE_PTR_OR_ERR_RET(instance, NULL); >> + RTE_PTR_OR_ERR_RET(instance->ops, NULL); >> + RTE_PTR_OR_ERR_RET(instance->ops->capabilities_get, NULL); >> return instance->ops->capabilities_get(instance->device); >> } >> >> @@ -113,7 +142,10 @@ rte_security_capability_get(struct rte_security_ctx >> *instance, >> const struct rte_security_capability *capability; >> uint16_t i = 0; >> >> - RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->capabilities_get, NULL); >> + RTE_PTR_OR_ERR_RET(instance, NULL); >> + RTE_PTR_OR_ERR_RET(instance->ops, NULL); >> + RTE_PTR_OR_ERR_RET(instance->ops->capabilities_get, NULL); >> + RTE_PTR_OR_ERR_RET(idx, NULL); >> capabilities = instance->ops->capabilities_get(instance->device); >> >> if (capabilities == NULL) >> @@ -121,7 +153,7 @@ rte_security_capability_get(struct rte_security_ctx >> *instance, >> >> while ((capability = &capabilities[i++])->action >> != RTE_SECURITY_ACTION_TYPE_NONE) { >> - if (capability->action == idx->action && >> + if (capability->action == idx->action && >> capability->protocol == idx->protocol) { >> if (idx->protocol == RTE_SECURITY_PROTOCOL_IPSEC) >> { >> if (capability->ipsec.proto == >> -- >> 2.17.1 -- Lukasz Wojciechowski Principal Software Engineer Samsung R&D Institute Poland Samsung Electronics Office +48 22 377 88 25 l.wojciechow@partner.samsung.com