diff mbox series

BUG: AddressSanitizer reports a buffer-overflow on rte_hash_lookup

Message ID CAC-fF8Qt6ExgN4zM7NeOGTHPvWk5xykWTYzEPKYm3tsYwj7EAg@mail.gmail.com (mailing list archive)
State Rejected, archived
Delegated to: Thomas Monjalon
Headers
Series BUG: AddressSanitizer reports a buffer-overflow on rte_hash_lookup |

Checks

Context Check Description
ci/loongarch-compilation warning apply patch failure
ci/iol-testing warning apply patch failure
ci/github-robot: build success github build: passed
ci/Intel-compilation warning apply issues

Commit Message

Isaac Boukris Feb. 5, 2023, 4:54 p.m. UTC 
  Hi,

I managed to reproduce it by modifying the helloworld app (see
attached). The report seem correct, as in case of 10 byte key the code
tries to look at the key as uint32 array and access k[2] which is two
bytes over, see:
https://github.com/DPDK/dpdk/blob/0bf5832222971a0154c9150d4a7a4b82ecbc9ddb/lib/hash/rte_jhash.h#L118

$ sudo build/helloworld --iova-mode=pa
EAL: Detected CPU lcores: 8
EAL: Detected NUMA nodes: 1
EAL: Detected static linkage of DPDK
EAL: Multi-process socket /var/run/dpdk/rte/mp_socket
EAL: Selected IOVA mode 'PA'
EAL: VFIO support initialized
EAL: Using IOMMU type 1 (Type 1)
EAL: Ignore mapping IO port bar(3)
EAL: Probe PCI driver: net_vmxnet3 (15ad:7b0) device: 0000:0b:00.0 (socket -1)
=================================================================
==21410==ERROR: AddressSanitizer: global-buffer-overflow on address
0x0000024fe428 at pc 0x000001293b0b bp 0x7fff126ef2d0 sp
0x7fff126ef2c0
READ of size 4 at 0x0000024fe428 thread T0
    #0 0x1293b0a in __rte_jhash_2hashes
(/home/admin/dpdk/share/dpdk/examples/helloworld/build/helloworld-static+0x1293b0a)
    #1 0x12953bf in rte_jhash_2hashes
(/home/admin/dpdk/share/dpdk/examples/helloworld/build/helloworld-static+0x12953bf)
    #2 0x12954c8 in rte_jhash
(/home/admin/dpdk/share/dpdk/examples/helloworld/build/helloworld-static+0x12954c8)
    #3 0x1bd7168 in rte_hash_lookup
(/home/admin/dpdk/share/dpdk/examples/helloworld/build/helloworld-static+0x1bd7168)
    #4 0x1295600 in main
(/home/admin/dpdk/share/dpdk/examples/helloworld/build/helloworld-static+0x1295600)
    #5 0x7fe8fffbbd84 in __libc_start_main (/lib64/libc.so.6+0x3ad84)
    #6 0x129356d in _start
(/home/admin/dpdk/share/dpdk/examples/helloworld/build/helloworld-static+0x129356d)

0x0000024fe42a is located 0 bytes to the right of global variable
'hash_key' defined in 'main.c:34:13' (0x24fe420) of size 10
SUMMARY: AddressSanitizer: global-buffer-overflow
(/home/admin/dpdk/share/dpdk/examples/helloworld/build/helloworld-static+0x1293b0a)
in __rte_jhash_2hashes

Comments

Stephen Hemminger Feb. 5, 2023, 7:49 p.m. UTC | #1 
On Sun, 5 Feb 2023 18:54:20 +0200
Isaac Boukris <iboukris@gmail.com> wrote:

> Hi,
> 
> I managed to reproduce it by modifying the helloworld app (see
> attached). The report seem correct, as in case of 10 byte key the code
> tries to look at the key as uint32 array and access k[2] which is two
> bytes over, see:
> https://github.com/DPDK/dpdk/blob/0bf5832222971a0154c9150d4a7a4b82ecbc9ddb/lib/hash/rte_jhash.h#L118
> 
> $ sudo build/helloworld --iova-mode=pa
> EAL: Detected CPU lcores: 8
> EAL: Detected NUMA nodes: 1
> EAL: Detected static linkage of DPDK
> EAL: Multi-process socket /var/run/dpdk/rte/mp_socket
> EAL: Selected IOVA mode 'PA'
> EAL: VFIO support initialized
> EAL: Using IOMMU type 1 (Type 1)
> EAL: Ignore mapping IO port bar(3)
> EAL: Probe PCI driver: net_vmxnet3 (15ad:7b0) device: 0000:0b:00.0 (socket -1)
> =================================================================
> ==21410==ERROR: AddressSanitizer: global-buffer-overflow on address
> 0x0000024fe428 at pc 0x000001293b0b bp 0x7fff126ef2d0 sp
> 0x7fff126ef2c0
> READ of size 4 at 0x0000024fe428 thread T0
>     #0 0x1293b0a in __rte_jhash_2hashes
> (/home/admin/dpdk/share/dpdk/examples/helloworld/build/helloworld-static+0x1293b0a)
>     #1 0x12953bf in rte_jhash_2hashes
> (/home/admin/dpdk/share/dpdk/examples/helloworld/build/helloworld-static+0x12953bf)
>     #2 0x12954c8 in rte_jhash
> (/home/admin/dpdk/share/dpdk/examples/helloworld/build/helloworld-static+0x12954c8)
>     #3 0x1bd7168 in rte_hash_lookup
> (/home/admin/dpdk/share/dpdk/examples/helloworld/build/helloworld-static+0x1bd7168)
>     #4 0x1295600 in main
> (/home/admin/dpdk/share/dpdk/examples/helloworld/build/helloworld-static+0x1295600)
>     #5 0x7fe8fffbbd84 in __libc_start_main (/lib64/libc.so.6+0x3ad84)
>     #6 0x129356d in _start
> (/home/admin/dpdk/share/dpdk/examples/helloworld/build/helloworld-static+0x129356d)
> 
> 0x0000024fe42a is located 0 bytes to the right of global variable
> 'hash_key' defined in 'main.c:34:13' (0x24fe420) of size 10
> SUMMARY: AddressSanitizer: global-buffer-overflow
> (/home/admin/dpdk/share/dpdk/examples/helloworld/build/helloworld-static+0x1293b0a)
> in __rte_jhash_2hashes

This code is using the common optimization of doing a full 32 bit access
and masking the result. This will read past the end of the passed input
but ignore the extra bytes. It won't be a problem unless the application
goes out of its way to put a hash key value at the end of a mapped
region.
Isaac Boukris Feb. 5, 2023, 8:14 p.m. UTC | #2 
On Sun, Feb 5, 2023 at 9:49 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Sun, 5 Feb 2023 18:54:20 +0200
> Isaac Boukris <iboukris@gmail.com> wrote:
>
> > Hi,
> >
> > I managed to reproduce it by modifying the helloworld app (see
> > attached). The report seem correct, as in case of 10 byte key the code
> > tries to look at the key as uint32 array and access k[2] which is two
> > bytes over, see:
> > https://github.com/DPDK/dpdk/blob/0bf5832222971a0154c9150d4a7a4b82ecbc9ddb/lib/hash/rte_jhash.h#L118
> >
> > $ sudo build/helloworld --iova-mode=pa
> > EAL: Detected CPU lcores: 8
> > EAL: Detected NUMA nodes: 1
> > EAL: Detected static linkage of DPDK
> > EAL: Multi-process socket /var/run/dpdk/rte/mp_socket
> > EAL: Selected IOVA mode 'PA'
> > EAL: VFIO support initialized
> > EAL: Using IOMMU type 1 (Type 1)
> > EAL: Ignore mapping IO port bar(3)
> > EAL: Probe PCI driver: net_vmxnet3 (15ad:7b0) device: 0000:0b:00.0 (socket -1)
> > =================================================================
> > ==21410==ERROR: AddressSanitizer: global-buffer-overflow on address
> > 0x0000024fe428 at pc 0x000001293b0b bp 0x7fff126ef2d0 sp
> > 0x7fff126ef2c0
> > READ of size 4 at 0x0000024fe428 thread T0
> >     #0 0x1293b0a in __rte_jhash_2hashes
> > (/home/admin/dpdk/share/dpdk/examples/helloworld/build/helloworld-static+0x1293b0a)
> >     #1 0x12953bf in rte_jhash_2hashes
> > (/home/admin/dpdk/share/dpdk/examples/helloworld/build/helloworld-static+0x12953bf)
> >     #2 0x12954c8 in rte_jhash
> > (/home/admin/dpdk/share/dpdk/examples/helloworld/build/helloworld-static+0x12954c8)
> >     #3 0x1bd7168 in rte_hash_lookup
> > (/home/admin/dpdk/share/dpdk/examples/helloworld/build/helloworld-static+0x1bd7168)
> >     #4 0x1295600 in main
> > (/home/admin/dpdk/share/dpdk/examples/helloworld/build/helloworld-static+0x1295600)
> >     #5 0x7fe8fffbbd84 in __libc_start_main (/lib64/libc.so.6+0x3ad84)
> >     #6 0x129356d in _start
> > (/home/admin/dpdk/share/dpdk/examples/helloworld/build/helloworld-static+0x129356d)
> >
> > 0x0000024fe42a is located 0 bytes to the right of global variable
> > 'hash_key' defined in 'main.c:34:13' (0x24fe420) of size 10
> > SUMMARY: AddressSanitizer: global-buffer-overflow
> > (/home/admin/dpdk/share/dpdk/examples/helloworld/build/helloworld-static+0x1293b0a)
> > in __rte_jhash_2hashes
>
> This code is using the common optimization of doing a full 32 bit access
> and masking the result. This will read past the end of the passed input
> but ignore the extra bytes. It won't be a problem unless the application
> goes out of its way to put a hash key value at the end of a mapped
> region.

Ack, fwiw it still makes it trickier to use AddressSanitizer in user app.
Stephen Hemminger Feb. 5, 2023, 9:08 p.m. UTC | #3 
On Sun, 5 Feb 2023 22:14:28 +0200
Isaac Boukris <iboukris@gmail.com> wrote:

> >
> > This code is using the common optimization of doing a full 32 bit access
> > and masking the result. This will read past the end of the passed input
> > but ignore the extra bytes. It won't be a problem unless the application
> > goes out of its way to put a hash key value at the end of a mapped
> > region.  
> 
> Ack, fwiw it still makes it trickier to use AddressSanitizer in user app.


Probably can be fixed with some annotations.
diff mbox series

Patch

From 44a74ac537fbee031bedda74fa05099f3fd3f552 Mon Sep 17 00:00:00 2001
From: Isaac Boukris <iboukris@gmail.com>
Date: Sun, 5 Feb 2023 11:20:29 +0200
Subject: [PATCH] Demo bug in rte_hash_lookup

---
 examples/helloworld/Makefile |  2 +-
 examples/helloworld/main.c   | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/examples/helloworld/Makefile b/examples/helloworld/Makefile
index 2a6a2f1527..14e44b8aa8 100644
--- a/examples/helloworld/Makefile
+++ b/examples/helloworld/Makefile
@@ -22,7 +22,7 @@  static: build/$(APP)-static
        ln -sf $(APP)-static build/$(APP)

 PC_FILE := $(shell $(PKGCONF) --path libdpdk 2>/dev/null)
-CFLAGS += -O3 $(shell $(PKGCONF) --cflags libdpdk)
+CFLAGS += -O0 -fsanitize=address $(shell $(PKGCONF) --cflags libdpdk)
 LDFLAGS_SHARED = $(shell $(PKGCONF) --libs libdpdk)
 LDFLAGS_STATIC = $(shell $(PKGCONF) --static --libs libdpdk)

diff --git a/examples/helloworld/main.c b/examples/helloworld/main.c
index af509138da..7460fbdfea 100644
--- a/examples/helloworld/main.c
+++ b/examples/helloworld/main.c
@@ -15,6 +15,11 @@ 
 #include <rte_lcore.h>
 #include <rte_debug.h>

+#include <assert.h>
+#include <rte_hash.h>
+#include <rte_fbk_hash.h>
+#include <rte_jhash.h>
+
 /* Launch a function on lcore. 8< */
 static int
 lcore_hello(__rte_unused void *arg)
@@ -26,18 +31,36 @@  lcore_hello(__rte_unused void *arg)
 }
 /* >8 End of launching function on lcore. */

+static char hash_key[10] = "";
+
+static struct rte_hash_parameters h_params = {
+       .entries = 64,
+       .key_len = sizeof(hash_key),
+       .hash_func = rte_jhash,
+       .hash_func_init_val = 0,
+       .socket_id = 0,
+};
+
 /* Initialization of Environment Abstraction Layer (EAL). 8< */
 int
 main(int argc, char **argv)
 {
        int ret;
        unsigned lcore_id;
+       struct rte_hash *handle;
+       int pos;

        ret = rte_eal_init(argc, argv);
        if (ret < 0)
                rte_panic("Cannot init EAL\n");
        /* >8 End of initialization of Environment Abstraction Layer */

+       handle = rte_hash_create(&h_params);
+       assert(handle != NULL);
+
+       pos = rte_hash_lookup(handle, &hash_key);
+       assert(pos == -ENOENT);
+
        /* Launches the function on each lcore. 8< */
        RTE_LCORE_FOREACH_WORKER(lcore_id) {
                /* Simpler equivalent. 8< */
--
2.31.1