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 <dev-boun...@dpdk.org> 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 <l.wojciec...@partner.samsung.com>
>> 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 <rte_malloc.h>
>> @@ -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.wojciec...@partner.samsung.com
