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

[Aaron Conole <aconole@redhat.com>]

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?