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 77D98A0577; Mon, 6 Apr 2020 20:49:06 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id D42682B96; Mon, 6 Apr 2020 20:49:05 +0200 (CEST) Received: from mailout1.w1.samsung.com (mailout1.w1.samsung.com [210.118.77.11]) by dpdk.org (Postfix) with ESMTP id 137DCF12 for ; Mon, 6 Apr 2020 20:49:03 +0200 (CEST) Received: from eucas1p1.samsung.com (unknown [182.198.249.206]) by mailout1.w1.samsung.com (KnoxPortal) with ESMTP id 20200406184902euoutp0181eab5f8b8a154df926b9d09281e97b7~DT4jVaGJ50807008070euoutp01q for ; Mon, 6 Apr 2020 18:49:02 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout1.w1.samsung.com 20200406184902euoutp0181eab5f8b8a154df926b9d09281e97b7~DT4jVaGJ50807008070euoutp01q DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1586198942; bh=vbo9BYQvTBX7lUgaicrcHmLYgVcqs0Yzx+UXR62Ap6U=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=ciq+54vfL/VjvGvm1ovEDQt8ZXxxANAZt/vtllka+bmlfUdHRBRDIQ50aWVk2uBwD r5sSIOBYizwr11e98mdioCiQeZKk7cq7ELdAoF0+usI2F0Gp3Zt5WS8e3xaQbfHzkp 6caouJ81C4wTn0H9piS2Dgr6Wue3P/UtI0ENdIlY= Received: from eusmges2new.samsung.com (unknown [203.254.199.244]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20200406184902eucas1p2738acb83d962b6db8e186366952009bb~DT4ioyCXy0620606206eucas1p2M; Mon, 6 Apr 2020 18:49:02 +0000 (GMT) Received: from eucas1p2.samsung.com ( [182.198.249.207]) by eusmges2new.samsung.com (EUCPMTA) with SMTP id 2C.09.60679.E997B8E5; Mon, 6 Apr 2020 19:49:02 +0100 (BST) Received: from eusmtrp2.samsung.com (unknown [182.198.249.139]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20200406184901eucas1p126d680c307ab6ee98c869e02a4981ed9~DT4iX4N240054600546eucas1p1c; Mon, 6 Apr 2020 18:49:01 +0000 (GMT) Received: from eusmgms1.samsung.com (unknown [182.198.249.179]) by eusmtrp2.samsung.com (KnoxPortal) with ESMTP id 20200406184901eusmtrp20e2851a140162c8763934861422255b9~DT4iXUWLB2873228732eusmtrp21; Mon, 6 Apr 2020 18:49:01 +0000 (GMT) X-AuditID: cbfec7f4-0e5ff7000001ed07-5b-5e8b799ea305 Received: from eusmtip2.samsung.com ( [203.254.199.222]) by eusmgms1.samsung.com (EUCPMTA) with SMTP id EB.43.08375.D997B8E5; Mon, 6 Apr 2020 19:49:01 +0100 (BST) Received: from [106.210.88.70] (unknown [106.210.88.70]) by eusmtip2.samsung.com (KnoxPortal) with ESMTPA id 20200406184901eusmtip2aa1ad2200344424877e1c4cdadaf1c3a~DT4h0TVm-1674716747eusmtip2L; Mon, 6 Apr 2020 18:49:01 +0000 (GMT) To: Anoob Joseph , "dev@dpdk.org" Cc: Narayana Prasad Raju Athreya , "Lukas Bartosik [C]" From: Lukasz Wojciechowski Message-ID: Date: Mon, 6 Apr 2020 20:49:00 +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: en-US X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFupjleLIzCtJLcpLzFFi42LZduzned15ld1xBq2XWCyWbdnKZPHu03Ym i5NH7rBY/HzZyu7A4vFrwVJWj8kLLzIHMEVx2aSk5mSWpRbp2yVwZXz485yx4ExSxYn1p1kb GLcGdTFyckgImEj0LbnE3sXIxSEksIJR4vThTiYI5wujxJfr31lAqoQEPjNK/HhTCtPx/OsF Noii5YwS664dYoRw3jJKNL09ywxSJSyQKLH+fztYt4iAi8S5V59YQWxmgXSJfU2/wWw2AVuJ IzO/gtm8Am4Sp15tBetlEVCR2Nh3lgnEFhWIlTj36AZUjaDEyZlPwGZyAsWfzP7IDjFTXqJ5 62xmCFtc4taT+WAvSAi0s0tsvNrNDHG2i8Tmf8tYIWxhiVfHt7BD2DIS/3fCNGxjlLj6+ycj hLOfUeJ67wqoKmuJw/9+Az3NAbRCU2L9Ln2IsKPE2pZJLCBhCQE+iRtvBSGO4JOYtG06M0SY V6KjTQiiWk/iac9URpi1f9Y+YZnAqDQLyWuzkLwzC8k7sxD2LmBkWcUonlpanJueWmyUl1qu V5yYW1yal66XnJ+7iRGYSk7/O/5lB+OuP0mHGAU4GJV4eCM4u+OEWBPLiitzDzFKcDArifBK 9XbGCfGmJFZWpRblxxeV5qQWH2KU5mBREuc1XvQyVkggPbEkNTs1tSC1CCbLxMEp1cC4uvPU 9f88yrPbI1hOaaj7fvA6tp/51PovPKcSxP6nNjM2cwt0bf9p7as+bZdNxpf+/Ac+9yujd3+P cl/Fs6d/0sSFO55tfmNXJbub/xKXsFyVoAzXhsu7LFcV3RJa0Za6QeioyIlsp9cbb3VtNi95 NkuyoZzti5afyubzv3xs9u44eoSdU/yYEktxRqKhFnNRcSIA5JiVJCEDAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrAIsWRmVeSWpSXmKPExsVy+t/xe7pzK7vjDLYcN7RYtmUrk8W7T9uZ LE4eucNi8fNlK7sDi8evBUtZPSYvvMgcwBSlZ1OUX1qSqpCRX1xiqxRtaGGkZ2hpoWdkYqln aGwea2VkqqRvZ5OSmpNZllqkb5egl/Hhz3PGgjNJFSfWn2ZtYNwa1MXIySEhYCLx/OsFti5G Lg4hgaWMEg+eH2bvYuQASshIfLgkAFEjLPHnWhdUzWtGiV23f7CCJIQFEiXW/29nAbFFBFwk zr36BBZnFkiXePj6GStEwzZmiVuL5rCDJNgEbCWOzPwKVsQr4CZx6tVWZhCbRUBFYmPfWSYQ W1QgVqK/eTcjRI2gxMmZT8AWcALFn8z+yA6xwExi3uaHzBC2vETz1tlQtrjErSfzmSYwCs1C 0j4LScssJC2zkLQsYGRZxSiSWlqcm55bbKhXnJhbXJqXrpecn7uJERg724793LyD8dLG4EOM AhyMSjy8EZzdcUKsiWXFlbmHGCU4mJVEeKV6O+OEeFMSK6tSi/Lji0pzUosPMZoCPTeRWUo0 OR8Y13kl8YamhuYWlobmxubGZhZK4rwdAgdjhATSE0tSs1NTC1KLYPqYODilGhgFb1mYVTb1 F25T2lm8/GiNhOeZD5c3Ff+SNsu88T78WqP01LWzrXcXu12U/m5qEsETYvbKq7x44berRa5a W25HdLjG87Jv3ji7yG/u81ru+2r82rwnbV7ZGfInmu1S0KoLZj+zIjr/xaaKMOOdTFOZ+qdc nPTcf7HbltuSq9ZWzduqa6eld0SJpTgj0VCLuag4EQDt7CzBswIAAA== X-CMS-MailID: 20200406184901eucas1p126d680c307ab6ee98c869e02a4981ed9 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] [EXT] Re: [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" Dear Anoob, Thank you for your reply and hints. Now I have patches ready to send as version 2, but I hesitate, because I don't like the idea of placing all checks in #ifdefs for RTE_LIBRTE_SECURITY_DEBUG config option. Let me explain here why: The config flag as a debug one will be disabled by default, which means, that normally nobody will use the checks. I believe that it is much better to have checks enabled as most of them will save the user of librte_security from segmentation faults, when trying to run instance->ops functions that are not supported or use invalid mempool object. I believe it will cause much less trouble to verify the error codes than to fight the segfault. It is also mentioned in the API description in few places that specific codes are returned in case some operation is not supported. Can we make such a changes in API, changing the current behavior from an error return code to segmentation fault during execution? That's why I would like to keep all of the checks enabled and not placed inside config option. However it would be nice to add the RTE_LIBRTE_SECURITY_DEBUG flag that you mentioned for changing checks behavior, to additionally provide logs of checks. This way a devloper using libret_security won't get a segmentation faults but error codes. If [s]he wants to check the details he'll rebuild the library with debug config option enabled and will be able to see all the details in logs, so [s]he will be able to fix the code. What do you think about such usage of the config debug flag? Best regards Lukasz W dniu 05.04.2020 o 14:54, Anoob Joseph pisze: > Hi Lukasz, > > Please see inline. > > Thanks, > Anoob > >> -----Original Message----- >> From: Lukasz Wojciechowski >> Sent: Saturday, April 4, 2020 12:06 AM >> To: Anoob Joseph ; dev@dpdk.org >> Cc: Narayana Prasad Raju Athreya ; Lukas Bartosik [C] >> >> Subject: [EXT] Re: [dpdk-dev] [PATCH 01/13] librte_security: fix verification of >> parameters >> >> External Email >> >> ---------------------------------------------------------------------- >> 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? > [Anoob] > > Check the below link. It explains the format of the patch with fixes. > https://protect2.fireeye.com/url?k=13e80549-4e769ea3-13e98e06-0cc47a6cba04-78f48282e990b416&q=1&u=https%3A%2F%2Fdoc.dpdk.org%2Fguides%2Fcontributing%2Fpatches.html%23commit-messages-body > >>>> 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=eee8020a-b37699e0-eee98945-0cc47a6cba04-561954f3424eceea&q=1&u=https%3A%2F%2Furldefense.proofpoint.com%2Fv2%2Furl%3Fu%3Dhttps-3A__protect2.fireeye. >>> com_url-3Fk-3Dc52d8c32-2D98e14097-2Dc52c077d-2D0cc47a30d446- >> 2Dc1b9d873 >>> e3e59cc4-26u-3Dhttp-3A__code.dpdk.org_dpdk_latest_source_lib_librte-5F >>> ethdev_rte-5Fethdev.h- >> 23L4372&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=jPfB >>> 8rwwviRSxyLWs2n6B- >> WYLn1v9SyTMrT5EQqh2TU&m=aTo18FDvqHQBghOAhbi7x0f6EuX7 >> wZHTUtsRRloZ9Bw&s=TXpv6uQZW1WwB_Av3vCaHeUaibQzA0ypUUqnPy5aQlE >> &e= >> Could you explain a bit further? >> >> Do you propose to make checks inside #ifdef RTE_LIBRTE_SECURITY_DEBUG or >> so? > [Anoob] Yes. You will need to introduce a new config flag (RTE_LIBRTE_SECURITY_DEBUG) and based on that, the error checks can be enabled/disabled. > >> And do you have all checks or just sess and m on mind? > [Anoob] I think we should have all checks under the config option. > >> The instance->ops->function checks were already there without any config >> options in all API functions. > [Anoob] Must have slipped through. Thanks for pointing it out. > >>>> + 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 -- Lukasz Wojciechowski Principal Software Engineer Samsung R&D Institute Poland Samsung Electronics Office +48 22 377 88 25 l.wojciechow@partner.samsung.com