* [dpdk-dev] [PATCH] test/hash: fix buffer overflow
@ 2021-10-08 21:28 Vladimir Medvedkin
2021-10-11 11:03 ` David Marchand
2021-10-13 19:27 ` [dpdk-dev] [PATCH v2] " Vladimir Medvedkin
0 siblings, 2 replies; 16+ messages in thread
From: Vladimir Medvedkin @ 2021-10-08 21:28 UTC (permalink / raw)
To: dev; +Cc: yipeng1.wang, sameh.gobriel, bruce.richardson, david.marchand, stable
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(-)
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);
--
2.7.4
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [dpdk-dev] [PATCH] test/hash: fix buffer overflow 2021-10-08 21:28 [dpdk-dev] [PATCH] test/hash: fix buffer overflow Vladimir Medvedkin @ 2021-10-11 11:03 ` David Marchand 2021-10-13 19:26 ` Medvedkin, Vladimir 2021-10-13 19:27 ` [dpdk-dev] [PATCH v2] " Vladimir Medvedkin 1 sibling, 1 reply; 16+ messages in thread From: David Marchand @ 2021-10-11 11:03 UTC (permalink / raw) To: Vladimir Medvedkin Cc: dev, Wang, Yipeng1, Gobriel, Sameh, Bruce Richardson, dpdk stable 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 ------- -- David Marchand ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH] test/hash: fix buffer overflow 2021-10-11 11:03 ` David Marchand @ 2021-10-13 19:26 ` Medvedkin, Vladimir 2021-10-14 7:04 ` David Marchand 0 siblings, 1 reply; 16+ messages in thread From: Medvedkin, Vladimir @ 2021-10-13 19:26 UTC (permalink / raw) To: David Marchand Cc: dev, Wang, Yipeng1, Gobriel, Sameh, Bruce Richardson, dpdk stable 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 > ------- > > -- Regards, Vladimir ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH] test/hash: fix buffer overflow 2021-10-13 19:26 ` Medvedkin, Vladimir @ 2021-10-14 7:04 ` David Marchand 2021-10-14 17:46 ` Medvedkin, Vladimir 0 siblings, 1 reply; 16+ messages in thread From: David Marchand @ 2021-10-14 7:04 UTC (permalink / raw) To: Medvedkin, Vladimir Cc: dev, Wang, Yipeng1, Gobriel, Sameh, Bruce Richardson, dpdk stable 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. -- David Marchand ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH] test/hash: fix buffer overflow 2021-10-14 7:04 ` David Marchand @ 2021-10-14 17:46 ` Medvedkin, Vladimir 0 siblings, 0 replies; 16+ messages in thread From: Medvedkin, Vladimir @ 2021-10-14 17:46 UTC (permalink / raw) To: David Marchand Cc: dev, Wang, Yipeng1, Gobriel, Sameh, Bruce Richardson, dpdk stable 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. > -- Regards, Vladimir ^ permalink raw reply [flat|nested] 16+ messages in thread
* [dpdk-dev] [PATCH v2] test/hash: fix buffer overflow 2021-10-08 21:28 [dpdk-dev] [PATCH] test/hash: fix buffer overflow Vladimir Medvedkin 2021-10-11 11:03 ` David Marchand @ 2021-10-13 19:27 ` Vladimir Medvedkin 2021-10-14 8:34 ` David Marchand 2021-10-14 17:48 ` [dpdk-dev] [PATCH v3] " Vladimir Medvedkin 1 sibling, 2 replies; 16+ messages in thread From: Vladimir Medvedkin @ 2021-10-13 19:27 UTC (permalink / raw) To: dev; +Cc: yipeng1.wang, sameh.gobriel, bruce.richardson, david.marchand, stable 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)); /* * 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); -- 2.7.4 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v2] test/hash: fix buffer overflow 2021-10-13 19:27 ` [dpdk-dev] [PATCH v2] " Vladimir Medvedkin @ 2021-10-14 8:34 ` David Marchand 2021-10-14 17:47 ` Medvedkin, Vladimir 2021-10-14 17:48 ` [dpdk-dev] [PATCH v3] " Vladimir Medvedkin 1 sibling, 1 reply; 16+ messages in thread From: David Marchand @ 2021-10-14 8:34 UTC (permalink / raw) To: Vladimir Medvedkin Cc: dev, Wang, Yipeng1, Gobriel, Sameh, Bruce Richardson, dpdk stable 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. -- David Marchand ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v2] test/hash: fix buffer overflow 2021-10-14 8:34 ` David Marchand @ 2021-10-14 17:47 ` Medvedkin, Vladimir 0 siblings, 0 replies; 16+ messages in thread From: Medvedkin, Vladimir @ 2021-10-14 17:47 UTC (permalink / raw) To: David Marchand Cc: dev, Wang, Yipeng1, Gobriel, Sameh, Bruce Richardson, dpdk stable 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [dpdk-dev] [PATCH v3] test/hash: fix buffer overflow 2021-10-13 19:27 ` [dpdk-dev] [PATCH v2] " Vladimir Medvedkin 2021-10-14 8:34 ` David Marchand @ 2021-10-14 17:48 ` Vladimir Medvedkin 2021-10-15 9:33 ` David Marchand 2021-10-21 7:40 ` David Marchand 1 sibling, 2 replies; 16+ messages in thread From: Vladimir Medvedkin @ 2021-10-14 17:48 UTC (permalink / raw) To: dev; +Cc: yipeng1.wang, sameh.gobriel, bruce.richardson, david.marchand, stable 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 adds a wrapper function around rte_jhash_32b() to reflect API differences regarding the length argument, effectively dividing it by 4. 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. 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 | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/app/test/test_hash.c b/app/test/test_hash.c index bd4d0cb..9daf99d 100644 --- a/app/test/test_hash.c +++ b/app/test/test_hash.c @@ -74,13 +74,17 @@ static uint32_t hashtest_key_lens[] = {0, 2, 4, 5, 6, 7, 8, 10, 11, 15, 16, 21, } \ } while (0) -/* 5-tuple key type */ +/* + * 5-tuple key type. + * Should be packed to avoid holes with potentially + * undefined content in the middle. + */ struct flow_key { uint32_t ip_src; uint32_t ip_dst; uint16_t port_src; uint16_t port_dst; - uint8_t proto; + uint32_t proto; } __rte_packed; /* @@ -1607,6 +1611,17 @@ static struct rte_hash_parameters hash_params_ex = { }; /* + * Wrapper function around rte_jhash_32b. + * It is required because rte_jhash_32b() accepts the length + * as size of 4-byte units. + */ +static inline uint32_t +test_jhash_32b(const void *k, uint32_t length, uint32_t initval) +{ + return rte_jhash_32b(k, length >> 2, initval); +} + +/* * add/delete key with jhash2 */ static int @@ -1618,7 +1633,7 @@ test_hash_add_delete_jhash2(void) hash_params_ex.name = "hash_test_jhash2"; hash_params_ex.key_len = 4; - hash_params_ex.hash_func = (rte_hash_function)rte_jhash_32b; + hash_params_ex.hash_func = (rte_hash_function)test_jhash_32b; handle = rte_hash_create(&hash_params_ex); if (handle == NULL) { @@ -1657,7 +1672,7 @@ test_hash_add_delete_2_jhash2(void) hash_params_ex.name = "hash_test_2_jhash2"; hash_params_ex.key_len = 8; - hash_params_ex.hash_func = (rte_hash_function)rte_jhash_32b; + hash_params_ex.hash_func = (rte_hash_function)test_jhash_32b; handle = rte_hash_create(&hash_params_ex); if (handle == NULL) @@ -2180,6 +2195,8 @@ test_hash_rcu_qsbr_sync_mode(uint8_t ext_bkt) static int test_hash(void) { + RTE_BUILD_BUG_ON(sizeof(struct flow_key) % sizeof(uint32_t) != 0); + if (test_add_delete() < 0) return -1; if (test_hash_add_delete_jhash2() < 0) -- 2.7.4 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test/hash: fix buffer overflow 2021-10-14 17:48 ` [dpdk-dev] [PATCH v3] " Vladimir Medvedkin @ 2021-10-15 9:33 ` David Marchand 2021-10-15 13:02 ` Medvedkin, Vladimir 2021-10-21 7:40 ` David Marchand 1 sibling, 1 reply; 16+ messages in thread From: David Marchand @ 2021-10-15 9:33 UTC (permalink / raw) To: Vladimir Medvedkin Cc: dev, Wang, Yipeng1, Gobriel, Sameh, Bruce Richardson, dpdk stable On Thu, Oct 14, 2021 at 7:55 PM Vladimir Medvedkin <vladimir.medvedkin@intel.com> wrote: > @@ -1607,6 +1611,17 @@ static struct rte_hash_parameters hash_params_ex = { > }; > > /* > + * Wrapper function around rte_jhash_32b. > + * It is required because rte_jhash_32b() accepts the length > + * as size of 4-byte units. > + */ > +static inline uint32_t > +test_jhash_32b(const void *k, uint32_t length, uint32_t initval) > +{ > + return rte_jhash_32b(k, length >> 2, initval); > +} I am confused. Does it mean that rte_jhash_32b is not compliant with rte_hash_create API? -- David Marchand ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test/hash: fix buffer overflow 2021-10-15 9:33 ` David Marchand @ 2021-10-15 13:02 ` Medvedkin, Vladimir 2021-10-19 7:02 ` David Marchand 0 siblings, 1 reply; 16+ messages in thread From: Medvedkin, Vladimir @ 2021-10-15 13:02 UTC (permalink / raw) To: David Marchand Cc: dev, Wang, Yipeng1, Gobriel, Sameh, Bruce Richardson, dpdk stable Hi David, On 15/10/2021 11:33, David Marchand wrote: > On Thu, Oct 14, 2021 at 7:55 PM Vladimir Medvedkin > <vladimir.medvedkin@intel.com> wrote: >> @@ -1607,6 +1611,17 @@ static struct rte_hash_parameters hash_params_ex = { >> }; >> >> /* >> + * Wrapper function around rte_jhash_32b. >> + * It is required because rte_jhash_32b() accepts the length >> + * as size of 4-byte units. >> + */ >> +static inline uint32_t >> +test_jhash_32b(const void *k, uint32_t length, uint32_t initval) >> +{ >> + return rte_jhash_32b(k, length >> 2, initval); >> +} > > I am confused. > Does it mean that rte_jhash_32b is not compliant with rte_hash_create API? > I think so too, because despite the fact that the ABI is the same, the API remains different with respect to the length argument. > -- Regards, Vladimir ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test/hash: fix buffer overflow 2021-10-15 13:02 ` Medvedkin, Vladimir @ 2021-10-19 7:02 ` David Marchand 2021-10-19 15:57 ` Medvedkin, Vladimir 0 siblings, 1 reply; 16+ messages in thread From: David Marchand @ 2021-10-19 7:02 UTC (permalink / raw) To: Medvedkin, Vladimir Cc: dev, Wang, Yipeng1, Gobriel, Sameh, Bruce Richardson, dpdk stable On Fri, Oct 15, 2021 at 3:02 PM Medvedkin, Vladimir <vladimir.medvedkin@intel.com> wrote: > > I am confused. > > Does it mean that rte_jhash_32b is not compliant with rte_hash_create API? > > > > I think so too, because despite the fact that the ABI is the same, the > API remains different with respect to the length argument. Sorry I don't follow you with "ABI is the same". Can you explain please? I am not against the fix, but it seems to test something different than what an application using the hash library would do. Or if an application directly calls this hash function, maybe the unit test should not test it via rte_hash_create (which seems to defeat the abstraction). -- David Marchand ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test/hash: fix buffer overflow 2021-10-19 7:02 ` David Marchand @ 2021-10-19 15:57 ` Medvedkin, Vladimir 2021-10-20 19:54 ` David Marchand 0 siblings, 1 reply; 16+ messages in thread From: Medvedkin, Vladimir @ 2021-10-19 15:57 UTC (permalink / raw) To: David Marchand Cc: dev, Wang, Yipeng1, Gobriel, Sameh, Bruce Richardson, dpdk stable Hi David, On 19/10/2021 09:02, David Marchand wrote: > On Fri, Oct 15, 2021 at 3:02 PM Medvedkin, Vladimir > <vladimir.medvedkin@intel.com> wrote: >>> I am confused. >>> Does it mean that rte_jhash_32b is not compliant with rte_hash_create API? >>> >> >> I think so too, because despite the fact that the ABI is the same, the >> API remains different with respect to the length argument. > > Sorry I don't follow you with "ABI is the same". > Can you explain please? > I meant that rte_hash accepts: /** Type of function that can be used for calculating the hash value. */ typedef uint32_t (*rte_hash_function)(const void *key, uint32_t key_len, uint32_t init_val); as a hash function. And signatures of rte_jhash() and rte_jhash_32b() are the same, but differ in the semantics of the "key_len" argument. Internally rte_hash passes a length of the key counted in bytes to this functions, so problems appears if configured hash function considers the key_len as something else than the size in bytes. > > I am not against the fix, but it seems to test something different > than what an application using the hash library would do. > Or if an application directly calls this hash function, maybe the unit > test should not test it via rte_hash_create (which seems to defeat the > abstraction). > I'd say that user should not use this hash function with rte_hash. Yipeng, Sameh, Bruce, what do you think? > -- Regards, Vladimir ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test/hash: fix buffer overflow 2021-10-19 15:57 ` Medvedkin, Vladimir @ 2021-10-20 19:54 ` David Marchand 2021-10-20 20:49 ` Wang, Yipeng1 0 siblings, 1 reply; 16+ messages in thread From: David Marchand @ 2021-10-20 19:54 UTC (permalink / raw) To: Wang, Yipeng1, Gobriel, Sameh, Bruce Richardson Cc: dev, dpdk stable, Medvedkin, Vladimir On Tue, Oct 19, 2021 at 5:58 PM Medvedkin, Vladimir <vladimir.medvedkin@intel.com> wrote: > > I am not against the fix, but it seems to test something different > > than what an application using the hash library would do. > > Or if an application directly calls this hash function, maybe the unit > > test should not test it via rte_hash_create (which seems to defeat the > > abstraction). > > > > I'd say that user should not use this hash function with rte_hash. > Yipeng, Sameh, Bruce, > what do you think? Guys, can we conclude? Thanks. -- David Marchand ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test/hash: fix buffer overflow 2021-10-20 19:54 ` David Marchand @ 2021-10-20 20:49 ` Wang, Yipeng1 0 siblings, 0 replies; 16+ messages in thread From: Wang, Yipeng1 @ 2021-10-20 20:49 UTC (permalink / raw) To: David Marchand, Gobriel, Sameh, Richardson, Bruce Cc: dev, dpdk stable, Medvedkin, Vladimir > -----Original Message----- > From: David Marchand <david.marchand@redhat.com> > Sent: Wednesday, October 20, 2021 12:54 PM > To: Wang, Yipeng1 <yipeng1.wang@intel.com>; Gobriel, Sameh > <sameh.gobriel@intel.com>; Richardson, Bruce > <bruce.richardson@intel.com> > Cc: dev <dev@dpdk.org>; dpdk stable <stable@dpdk.org>; Medvedkin, > Vladimir <vladimir.medvedkin@intel.com> > Subject: Re: [PATCH v3] test/hash: fix buffer overflow > > On Tue, Oct 19, 2021 at 5:58 PM Medvedkin, Vladimir > <vladimir.medvedkin@intel.com> wrote: > > > I am not against the fix, but it seems to test something different > > > than what an application using the hash library would do. > > > Or if an application directly calls this hash function, maybe the > > > unit test should not test it via rte_hash_create (which seems to > > > defeat the abstraction). > > > > > > > I'd say that user should not use this hash function with rte_hash. > > Yipeng, Sameh, Bruce, > > what do you think? > > Guys, can we conclude? > Thanks. > > > -- > David Marchand [Wang, Yipeng] Vladimir, Bruce and I discussed a bit offline and I think we agreed on this fix. But we should be more clear in the comment of rte_hash_create about what hash function the API expected (I.e. key length is byte count). Acked-by: Yipeng Wang <yipeng1.wang@intel.com> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test/hash: fix buffer overflow 2021-10-14 17:48 ` [dpdk-dev] [PATCH v3] " Vladimir Medvedkin 2021-10-15 9:33 ` David Marchand @ 2021-10-21 7:40 ` David Marchand 1 sibling, 0 replies; 16+ messages in thread From: David Marchand @ 2021-10-21 7:40 UTC (permalink / raw) To: Vladimir Medvedkin Cc: dev, Wang, Yipeng1, Gobriel, Sameh, Bruce Richardson, dpdk stable On Thu, Oct 14, 2021 at 7:55 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 adds a wrapper function around rte_jhash_32b() > to reflect API differences regarding the length argument, > effectively dividing it by 4. > > 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. > > Bugzilla ID: 818 > Fixes: af75078fece3 ("first public release") > Cc: stable@dpdk.org > > Signed-off-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com> Acked-by: Yipeng Wang <yipeng1.wang@intel.com> Removed a few comments in code (about previous size of flow_key struct), and applied. Thanks. -- David Marchand ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-10-21 7:41 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-08 21:28 [dpdk-dev] [PATCH] test/hash: fix buffer overflow Vladimir Medvedkin 2021-10-11 11:03 ` David Marchand 2021-10-13 19:26 ` Medvedkin, Vladimir 2021-10-14 7:04 ` David Marchand 2021-10-14 17:46 ` Medvedkin, Vladimir 2021-10-13 19:27 ` [dpdk-dev] [PATCH v2] " Vladimir Medvedkin 2021-10-14 8:34 ` David Marchand 2021-10-14 17:47 ` Medvedkin, Vladimir 2021-10-14 17:48 ` [dpdk-dev] [PATCH v3] " Vladimir Medvedkin 2021-10-15 9:33 ` David Marchand 2021-10-15 13:02 ` Medvedkin, Vladimir 2021-10-19 7:02 ` David Marchand 2021-10-19 15:57 ` Medvedkin, Vladimir 2021-10-20 19:54 ` David Marchand 2021-10-20 20:49 ` Wang, Yipeng1 2021-10-21 7:40 ` David Marchand
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).