diff mbox series

[v2] test/hash: fix buffer overflow

Message ID 1634153265-193315-1-git-send-email-vladimir.medvedkin@intel.com (mailing list archive)
State Superseded
Delegated to: David Marchand
Headers show
Series [v2] test/hash: fix buffer overflow | expand

Checks

Context Check Description
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/intel-Testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-mellanox-Performance fail Performance Testing issues
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/checkpatch success coding style OK

Commit Message

Vladimir Medvedkin Oct. 13, 2021, 7:27 p.m. UTC
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(-)

Comments

David Marchand Oct. 14, 2021, 8:34 a.m. UTC | #1
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.
Vladimir Medvedkin Oct. 14, 2021, 5:47 p.m. UTC | #2
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.

>
diff mbox series

Patch

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));
 
 /*
  * Hash function that always returns the same value, to easily test what
@@ -1617,7 +1617,8 @@  test_hash_add_delete_jhash2(void)
 	int32_t pos1, pos2;
 
 	hash_params_ex.name = "hash_test_jhash2";
-	hash_params_ex.key_len = 4;
+	/* Set the key_len divided by 4 due to using rte_jhash_32b() */
+	hash_params_ex.key_len = 4 / sizeof(uint32_t);
 	hash_params_ex.hash_func = (rte_hash_function)rte_jhash_32b;
 
 	handle = rte_hash_create(&hash_params_ex);
@@ -1656,7 +1657,8 @@  test_hash_add_delete_2_jhash2(void)
 	int32_t pos1, pos2;
 
 	hash_params_ex.name = "hash_test_2_jhash2";
-	hash_params_ex.key_len = 8;
+	/* Set the key_len divided by 4 due to using rte_jhash_32b() */
+	hash_params_ex.key_len = 8 / sizeof(uint32_t);
 	hash_params_ex.hash_func = (rte_hash_function)rte_jhash_32b;
 
 	handle = rte_hash_create(&hash_params_ex);