Re: [dpdk-stable] [PATCH v2] test/hash: fix buffer overflow

["Medvedkin, Vladimir" <vladimir.medvedkin@intel.com>]

Hi David,

On 14/10/2021 10:34, David Marchand wrote:
> On Wed, Oct 13, 2021 at 9:28 PM Vladimir Medvedkin
> <vladimir.medvedkin@intel.com> wrote:
>>
>> This patch fixes buffer overflow reported by ASAN,
>> please reference https://bugs.dpdk.org/show_bug.cgi?id=818
>>
>> Some tests for the rte_hash table use the rte_jhash_32b() as
>> the hash function. This hash function interprets the length
>> argument in units of 4 bytes.
>>
>> This patch divides configured key length by 4 in cases when
>> rte_jhash_32b() is used.
>>
>> For some tests rte_jhash() is used with keys of length not
>> a multiple of 4 bytes. From the rte_jhash() documentation:
>> If input key is not aligned to four byte boundaries or a
>> multiple of four bytes in length, the memory region just
>> after may be read (but not used in the computation).
>>
>> This patch increases the size of the proto field of the
>> flow_key struct up to uint32_t and sets the alignment to 4 bytes.
>>
>> Bugzilla ID: 818
>> Fixes: af75078fece3 ("first public release")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
>> ---
>>   app/test/test_hash.c | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/app/test/test_hash.c b/app/test/test_hash.c
>> index bd4d0cb..e3f2d29 100644
>> --- a/app/test/test_hash.c
>> +++ b/app/test/test_hash.c
>> @@ -80,8 +80,8 @@ struct flow_key {
>>          uint32_t ip_dst;
>>          uint16_t port_src;
>>          uint16_t port_dst;
>> -       uint8_t proto;
>> -} __rte_packed;
>> +       uint32_t proto;
>> +} __rte_packed __rte_aligned(sizeof(uint32_t));
> 
> If in the future, we add a field not multiple of sizeof(uint32_t),
> there will be some padding at the end of the structure.
> I *think* holes and padding content is undefined for initialized
> objects (though maybe things could be different with objects in .data
> ?).
> That's probably something to confirm.
> If this is the case, the hash function would consider random data.
> 
> I think growing the proto field to uint32_t like you did is the right
> fix since the whole structure is now naturally uint32_t aligned.
> 
> But I would remove the aligned attribute and prefer
> RTE_BUILD_BUG(sizeof(struct flow_key) % sizeof(sizeof(uint32_t)) !=
> 0).
> Maybe add a comment to explain we keep the packed attribute to avoid
> holes with potentially undefined content in the middle of this struct.
> 

Agree, will do in v3.

> 

-- 
Regards,
Vladimir