Ferruh Yigit <ferruh.yi...@intel.com> writes:
> On 7/30/2019 4:43 PM, Aaron Conole wrote:
>> Ferruh Yigit <ferruh.yi...@intel.com> writes:
>>
>>> On 7/30/2019 3:42 PM, Aaron Conole wrote:
>>>> David Marchand <david.march...@redhat.com> writes:
>>>>
>>>>> On Wed, Jul 10, 2019 at 11:49 PM Thomas Monjalon <tho...@monjalon.net>
>>>>> 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: sta...@dpdk.org
>>>>>>> Signed-off-by: Bernard Iremonger <bernard.iremon...@intel.com>
>>>>>>
>>>>>> 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 <acon...@redhat.com>
>> ---
>> 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
