Re: [dpdk-dev] [dpdk-stable] [PATCH] librte_flow_classify: fix out-of-bounds access

[Aaron Conole <aconole@redhat.com>]

Ferruh Yigit <ferruh.yigit@intel.com> writes:

> On 7/30/2019 4:43 PM, Aaron Conole wrote:
>> Ferruh Yigit <ferruh.yigit@intel.com> writes:
>> 
>>> On 7/30/2019 3:42 PM, Aaron Conole wrote:
>>>> David Marchand <david.marchand@redhat.com> writes:
>>>>
>>>>> On Wed, Jul 10, 2019 at 11:49 PM Thomas Monjalon <thomas@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: stable@dpdk.org
>>>>>>> Signed-off-by: Bernard Iremonger <bernard.iremonger@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 <aconole@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