diff mbox series

test/hash: fix buffer overflow

Message ID 1633728526-197782-1-git-send-email-vladimir.medvedkin@intel.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series test/hash: fix buffer overflow |

Checks

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

Commit Message

Vladimir Medvedkin Oct. 8, 2021, 9:28 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.

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 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

David Marchand Oct. 11, 2021, 11:03 a.m. UTC | #1 
On Fri, Oct 8, 2021 at 11: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.
>
> Bugzilla ID: 818
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
>

With patch applied, ASan reports another issue.
Did you test your fix with ASan?

From GHA, with https://patchwork.dpdk.org/project/dpdk/patch/20211002162432.4348-4-david.marchand@redhat.com/
applied:


30/94 DPDK:fast-tests / hash_autotest         FAIL     0.87 s (exit status 1)

--- command ---
DPDK_TEST='hash_autotest'
/home/runner/work/dpdk/dpdk/build/app/test/dpdk-test -l 0-1
--file-prefix=hash_autotest
--- stdout ---
RTE>>hash_autotest
--- stderr ---
EAL: Detected CPU lcores: 2
EAL: Detected NUMA nodes: 1
EAL: Detected shared linkage of DPDK
EAL: WARNING! Base virtual address hint (0x100005000 !=
0x7fa4a7cda000) not respected!
EAL:    This may cause issues with mapping memory into secondary processes
EAL: Multi-process socket /var/run/dpdk/hash_autotest/mp_socket
EAL: Selected IOVA mode 'PA'
EAL: No available 1048576 kB hugepages reported
EAL: VFIO support initialized
EAL: WARNING! Base virtual address hint (0x10000b000 !=
0x7fa49688f000) not respected!
EAL:    This may cause issues with mapping memory into secondary processes
EAL: WARNING! Base virtual address hint (0x100011000 !=
0x7fa49682e000) not respected!
EAL:    This may cause issues with mapping memory into secondary processes
EAL: WARNING! Base virtual address hint (0x100a12000 !=
0x7fa094a00000) not respected!
EAL:    This may cause issues with mapping memory into secondary processes
EAL: WARNING! Base virtual address hint (0x100c17000 !=
0x7fa49669f000) not respected!
EAL:    This may cause issues with mapping memory into secondary processes
EAL: WARNING! Base virtual address hint (0x101618000 !=
0x7f9c94800000) not respected!
EAL:    This may cause issues with mapping memory into secondary processes
EAL: WARNING! Base virtual address hint (0x10181d000 !=
0x7fa49663e000) not respected!
EAL:    This may cause issues with mapping memory into secondary processes
EAL: WARNING! Base virtual address hint (0x10221e000 !=
0x7f9894600000) not respected!
EAL:    This may cause issues with mapping memory into secondary processes
EAL: WARNING! Base virtual address hint (0x102423000 !=
0x7fa49649f000) not respected!
EAL:    This may cause issues with mapping memory into secondary processes
EAL: WARNING! Base virtual address hint (0x102e24000 !=
0x7f9494400000) not respected!
EAL:    This may cause issues with mapping memory into secondary processes
APP: HPET is not enabled, using TSC as default timer
=================================================================
==26840==ERROR: AddressSanitizer: global-buffer-overflow on address
0x00000372e3e0 at pc 0x0000014b0eb8 bp 0x7fff80e49990 sp
0x7fff80e49988
READ of size 4 at 0x00000372e3e0 thread T0
    #0 0x14b0eb7 in __rte_jhash_2hashes
/home/runner/work/dpdk/dpdk/build/../lib/hash/rte_jhash.h:137:9
    #1 0x14b0130 in rte_jhash_2hashes
/home/runner/work/dpdk/dpdk/build/../lib/hash/rte_jhash.h:238:2
    #2 0x14b0051 in rte_jhash
/home/runner/work/dpdk/dpdk/build/../lib/hash/rte_jhash.h:284:2
    #3 0x7fa4a38c7627 in rte_hash_hash
/home/runner/work/dpdk/dpdk/build/../lib/hash/rte_cuckoo_hash.c:538:9
    #4 0x7fa4a38d6672 in rte_hash_add_key
/home/runner/work/dpdk/dpdk/build/../lib/hash/rte_cuckoo_hash.c:1212:46
    #5 0x14a06db in test_five_keys
/home/runner/work/dpdk/dpdk/build/../app/test/test_hash.c:715:12
    #6 0x149deda in test_hash
/home/runner/work/dpdk/dpdk/build/../app/test/test_hash.c:2207:6
    #7 0x4d61f6 in cmd_autotest_parsed
/home/runner/work/dpdk/dpdk/build/../app/test/commands.c:71:10
    #8 0x7fa4a44356c5 in cmdline_parse
/home/runner/work/dpdk/dpdk/build/../lib/cmdline/cmdline_parse.c:290:3
    #9 0x7fa4a442e8d5 in cmdline_valid_buffer
/home/runner/work/dpdk/dpdk/build/../lib/cmdline/cmdline.c:26:8
    #10 0x7fa4a443ff07 in rdline_char_in
/home/runner/work/dpdk/dpdk/build/../lib/cmdline/cmdline_rdline.c:421:5
    #11 0x7fa4a442f03f in cmdline_in
/home/runner/work/dpdk/dpdk/build/../lib/cmdline/cmdline.c:149:9
    #12 0x5ac71e in main
/home/runner/work/dpdk/dpdk/build/../app/test/test.c:214:8
    #13 0x7fa49ca42bf6 in __libc_start_main
/build/glibc-S9d2JN/glibc-2.27/csu/../csu/libc-start.c:310
    #14 0x42eaa9 in _start
(/home/runner/work/dpdk/dpdk/build/app/test/dpdk-test+0x42eaa9)

0x00000372e3e1 is located 0 bytes to the right of global variable
'keys' defined in '../app/test/test_hash.c:115:24' (0x372e3a0) of size
65
SUMMARY: AddressSanitizer: global-buffer-overflow
/home/runner/work/dpdk/dpdk/build/../lib/hash/rte_jhash.h:137:9 in
__rte_jhash_2hashes
Shadow bytes around the buggy address:
  0x0000806ddc20: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x0000806ddc30: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x0000806ddc40: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x0000806ddc50: f9 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 f9 f9 f9 f9
  0x0000806ddc60: 01 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 00 00 f9 f9
=>0x0000806ddc70: f9 f9 f9 f9 00 00 00 00 00 00 00 00[01]f9 f9 f9
  0x0000806ddc80: f9 f9 f9 f9 00 00 00 00 00 00 f9 f9 f9 f9 f9 f9
  0x0000806ddc90: 00 00 f9 f9 f9 f9 f9 f9 00 00 f9 f9 f9 f9 f9 f9
  0x0000806ddca0: 00 00 00 00 00 00 00 00 04 f9 f9 f9 f9 f9 f9 f9
  0x0000806ddcb0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x0000806ddcc0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==26840==ABORTING
-------
Vladimir Medvedkin Oct. 13, 2021, 7:26 p.m. UTC | #2 
Hi David,

On 11/10/2021 13:03, David Marchand wrote:
> On Fri, Oct 8, 2021 at 11: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.
>>
>> Bugzilla ID: 818
>> Fixes: af75078fece3 ("first public release")
>> Cc: stable@dpdk.org
>>
> 
> With patch applied, ASan reports another issue.
> Did you test your fix with ASan?
> 

You're right, for some reason ASAN wasn't enabled.
I applied patch and built running .ci/linux-build.sh,
also I build with CFLAGS + LDFLAGS.

Bruce suggested to use meson options instead of using CFLAGS, so
meson configure build -Db_sanitize=address -Db_lundef=false
works fine.

I'll sent v2 for this.

>  From GHA, with https://patchwork.dpdk.org/project/dpdk/patch/20211002162432.4348-4-david.marchand@redhat.com/
> applied:
> 
> 
> 30/94 DPDK:fast-tests / hash_autotest         FAIL     0.87 s (exit status 1)
> 
> --- command ---
> DPDK_TEST='hash_autotest'
> /home/runner/work/dpdk/dpdk/build/app/test/dpdk-test -l 0-1
> --file-prefix=hash_autotest
> --- stdout ---
> RTE>>hash_autotest
> --- stderr ---
> EAL: Detected CPU lcores: 2
> EAL: Detected NUMA nodes: 1
> EAL: Detected shared linkage of DPDK
> EAL: WARNING! Base virtual address hint (0x100005000 !=
> 0x7fa4a7cda000) not respected!
> EAL:    This may cause issues with mapping memory into secondary processes
> EAL: Multi-process socket /var/run/dpdk/hash_autotest/mp_socket
> EAL: Selected IOVA mode 'PA'
> EAL: No available 1048576 kB hugepages reported
> EAL: VFIO support initialized
> EAL: WARNING! Base virtual address hint (0x10000b000 !=
> 0x7fa49688f000) not respected!
> EAL:    This may cause issues with mapping memory into secondary processes
> EAL: WARNING! Base virtual address hint (0x100011000 !=
> 0x7fa49682e000) not respected!
> EAL:    This may cause issues with mapping memory into secondary processes
> EAL: WARNING! Base virtual address hint (0x100a12000 !=
> 0x7fa094a00000) not respected!
> EAL:    This may cause issues with mapping memory into secondary processes
> EAL: WARNING! Base virtual address hint (0x100c17000 !=
> 0x7fa49669f000) not respected!
> EAL:    This may cause issues with mapping memory into secondary processes
> EAL: WARNING! Base virtual address hint (0x101618000 !=
> 0x7f9c94800000) not respected!
> EAL:    This may cause issues with mapping memory into secondary processes
> EAL: WARNING! Base virtual address hint (0x10181d000 !=
> 0x7fa49663e000) not respected!
> EAL:    This may cause issues with mapping memory into secondary processes
> EAL: WARNING! Base virtual address hint (0x10221e000 !=
> 0x7f9894600000) not respected!
> EAL:    This may cause issues with mapping memory into secondary processes
> EAL: WARNING! Base virtual address hint (0x102423000 !=
> 0x7fa49649f000) not respected!
> EAL:    This may cause issues with mapping memory into secondary processes
> EAL: WARNING! Base virtual address hint (0x102e24000 !=
> 0x7f9494400000) not respected!
> EAL:    This may cause issues with mapping memory into secondary processes
> APP: HPET is not enabled, using TSC as default timer
> =================================================================
> ==26840==ERROR: AddressSanitizer: global-buffer-overflow on address
> 0x00000372e3e0 at pc 0x0000014b0eb8 bp 0x7fff80e49990 sp
> 0x7fff80e49988
> READ of size 4 at 0x00000372e3e0 thread T0
>      #0 0x14b0eb7 in __rte_jhash_2hashes
> /home/runner/work/dpdk/dpdk/build/../lib/hash/rte_jhash.h:137:9
>      #1 0x14b0130 in rte_jhash_2hashes
> /home/runner/work/dpdk/dpdk/build/../lib/hash/rte_jhash.h:238:2
>      #2 0x14b0051 in rte_jhash
> /home/runner/work/dpdk/dpdk/build/../lib/hash/rte_jhash.h:284:2
>      #3 0x7fa4a38c7627 in rte_hash_hash
> /home/runner/work/dpdk/dpdk/build/../lib/hash/rte_cuckoo_hash.c:538:9
>      #4 0x7fa4a38d6672 in rte_hash_add_key
> /home/runner/work/dpdk/dpdk/build/../lib/hash/rte_cuckoo_hash.c:1212:46
>      #5 0x14a06db in test_five_keys
> /home/runner/work/dpdk/dpdk/build/../app/test/test_hash.c:715:12
>      #6 0x149deda in test_hash
> /home/runner/work/dpdk/dpdk/build/../app/test/test_hash.c:2207:6
>      #7 0x4d61f6 in cmd_autotest_parsed
> /home/runner/work/dpdk/dpdk/build/../app/test/commands.c:71:10
>      #8 0x7fa4a44356c5 in cmdline_parse
> /home/runner/work/dpdk/dpdk/build/../lib/cmdline/cmdline_parse.c:290:3
>      #9 0x7fa4a442e8d5 in cmdline_valid_buffer
> /home/runner/work/dpdk/dpdk/build/../lib/cmdline/cmdline.c:26:8
>      #10 0x7fa4a443ff07 in rdline_char_in
> /home/runner/work/dpdk/dpdk/build/../lib/cmdline/cmdline_rdline.c:421:5
>      #11 0x7fa4a442f03f in cmdline_in
> /home/runner/work/dpdk/dpdk/build/../lib/cmdline/cmdline.c:149:9
>      #12 0x5ac71e in main
> /home/runner/work/dpdk/dpdk/build/../app/test/test.c:214:8
>      #13 0x7fa49ca42bf6 in __libc_start_main
> /build/glibc-S9d2JN/glibc-2.27/csu/../csu/libc-start.c:310
>      #14 0x42eaa9 in _start
> (/home/runner/work/dpdk/dpdk/build/app/test/dpdk-test+0x42eaa9)
> 
> 0x00000372e3e1 is located 0 bytes to the right of global variable
> 'keys' defined in '../app/test/test_hash.c:115:24' (0x372e3a0) of size
> 65
> SUMMARY: AddressSanitizer: global-buffer-overflow
> /home/runner/work/dpdk/dpdk/build/../lib/hash/rte_jhash.h:137:9 in
> __rte_jhash_2hashes
> Shadow bytes around the buggy address:
>    0x0000806ddc20: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
>    0x0000806ddc30: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
>    0x0000806ddc40: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
>    0x0000806ddc50: f9 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 f9 f9 f9 f9
>    0x0000806ddc60: 01 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 00 00 f9 f9
> =>0x0000806ddc70: f9 f9 f9 f9 00 00 00 00 00 00 00 00[01]f9 f9 f9
>    0x0000806ddc80: f9 f9 f9 f9 00 00 00 00 00 00 f9 f9 f9 f9 f9 f9
>    0x0000806ddc90: 00 00 f9 f9 f9 f9 f9 f9 00 00 f9 f9 f9 f9 f9 f9
>    0x0000806ddca0: 00 00 00 00 00 00 00 00 04 f9 f9 f9 f9 f9 f9 f9
>    0x0000806ddcb0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
>    0x0000806ddcc0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
> Shadow byte legend (one shadow byte represents 8 application bytes):
>    Addressable:           00
>    Partially addressable: 01 02 03 04 05 06 07
>    Heap left redzone:       fa
>    Freed heap region:       fd
>    Stack left redzone:      f1
>    Stack mid redzone:       f2
>    Stack right redzone:     f3
>    Stack after return:      f5
>    Stack use after scope:   f8
>    Global redzone:          f9
>    Global init order:       f6
>    Poisoned by user:        f7
>    Container overflow:      fc
>    Array cookie:            ac
>    Intra object redzone:    bb
>    ASan internal:           fe
>    Left alloca redzone:     ca
>    Right alloca redzone:    cb
>    Shadow gap:              cc
> ==26840==ABORTING
> -------
> 
>
David Marchand Oct. 14, 2021, 7:04 a.m. UTC | #3 
Hello Vladimir,

On Wed, Oct 13, 2021 at 9:27 PM Medvedkin, Vladimir
<vladimir.medvedkin@intel.com> wrote:
> > With patch applied, ASan reports another issue.
> > Did you test your fix with ASan?
> >
>
> You're right, for some reason ASAN wasn't enabled.
> I applied patch and built running .ci/linux-build.sh,
> also I build with CFLAGS + LDFLAGS.
>
> Bruce suggested to use meson options instead of using CFLAGS, so
> meson configure build -Db_sanitize=address -Db_lundef=false
> works fine.

Well, yes, you can directly do this.
I linked to my GHA patch in the bz, because I find it easier and
reproducible to push fixes in GHA and get the result: no question
about "did I enable ASan?" or "did I start the test correctly?".

FYI, b_lundef seems necessary only with clang, gcc should be fine without it.
IIUC, those compilers went with different choices on how to pull
libasan (clang went with static, gcc went with shared).
Hopefully, we will have something easier to use in DPDK with Zhihong work.

>
> I'll sent v2 for this.

Thanks, I'll look at it.
Vladimir Medvedkin Oct. 14, 2021, 5:46 p.m. UTC | #4 
Hi David,

On 14/10/2021 09:04, David Marchand wrote:
> Hello Vladimir,
> 
> On Wed, Oct 13, 2021 at 9:27 PM Medvedkin, Vladimir
> <vladimir.medvedkin@intel.com> wrote:
>>> With patch applied, ASan reports another issue.
>>> Did you test your fix with ASan?
>>>
>>
>> You're right, for some reason ASAN wasn't enabled.
>> I applied patch and built running .ci/linux-build.sh,
>> also I build with CFLAGS + LDFLAGS.
>>
>> Bruce suggested to use meson options instead of using CFLAGS, so
>> meson configure build -Db_sanitize=address -Db_lundef=false
>> works fine.
> 
> Well, yes, you can directly do this.
> I linked to my GHA patch in the bz, because I find it easier and
> reproducible to push fixes in GHA and get the result: no question
> about "did I enable ASan?" or "did I start the test correctly?".
> 
> FYI, b_lundef seems necessary only with clang, gcc should be fine without it.
> IIUC, those compilers went with different choices on how to pull
> libasan (clang went with static, gcc went with shared).
> Hopefully, we will have something easier to use in DPDK with Zhihong work.
> 

Thanks!

>>
>> I'll sent v2 for this.
> 
> Thanks, I'll look at it.
> 

I'm going to send v3, because just dividing the size of the key for 
jhash_32b() cases is not correct (because rte_hash will compare just a 
part of the key in this case), so I'll replace rte_jhash_32b with a 
wrapper function.

>
diff mbox series

Patch

diff --git a/app/test/test_hash.c b/app/test/test_hash.c
index bd4d0cb..650d977 100644
--- a/app/test/test_hash.c
+++ b/app/test/test_hash.c
@@ -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);