Skip to contentSkip to conversation

eal: Copy raw strings taken from command line

PATCH Applied Patrick MacArthur 3 participants 3 messages Aug 4, 2017 – Oct 9, 2017

lore
Share & tools
lore

Patrick MacArthur

author
#0 lore raw

PATCH 1/1 eal: Copy raw strings taken from command line

Normally, command line argument strings are considered immutable, but SPDK [1] and urdma [2] construct argv arrays to pass to rte_eal_init(). These strings are allocated using malloc() and freed after DPDK initialization with free(). However, in the case of --file-prefix and --huge-dir, DPDK takes the pointer to these strings in argv directly. If a secondary process calls rte_eal_pci_probe() after rte_eal_init() returns, as is done by SPDK, this causes a use-after-free error because the strings have been freed by the calling code immediately after rte_eal_init() returns.

This problem was observed when running SPDK example programs as a secondary process and causes the secondary processes to fail:

Quoted 11 lines

Starting DPDK 16.11.1 initialization...
[ DPDK EAL parameters: identify -c 4 --file-prefix=spdk3260 --base-virtaddr=0x1000000000 --proc-type=auto ]

EAL: Detected 40 lcore(s)
EAL: Auto-detected process type: SECONDARY
EAL: Probing VFIO support...
EAL: VFIO support initialized
EAL: PCI device 0000:81:00.0 on NUMA socket 1
EAL:   probe driver: 8086:953 spdk_nvme
EAL:   cannot connect to primary process!
EAL: Error - exiting with code: 1
Cause: Requested device 0000:81:00.0 cannot be used

Running strace shows that the file prefix has been zero'd out by the time that the secondary process attempts to probe the NVMe device.

The use-after-free errors can be easily detected with valgrind:

Quoted 24 lines

==8489== Invalid read of size 1

==8489==    at 0x4C30D22: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8489==    by 0x58DB955: vfprintf (vfprintf.c:1637)
==8489==    by 0x59A4685: __vsnprintf_chk (vsnprintf_chk.c:63)
==8489==    by 0x59A45E7: __snprintf_chk (snprintf_chk.c:34)
==8489==    by 0x1246AB: get_socket_path.constprop.0 (in /home/pmacarth/src/spdk/examples/nvme/identify/identify)
==8489==    by 0x124B09: vfio_mp_sync_connect_to_primary (in /home/pmacarth/src/spdk/examples/nvme/identify/identify)
==8489==    by 0x123BE4: vfio_get_group_fd.part.1 (in /home/pmacarth/src/spdk/examples/nvme/identify/identify)
==8489==    by 0x124366: vfio_setup_device (in /home/pmacarth/src/spdk/examples/nvme/identify/identify)
==8489==    by 0x126C8A: pci_vfio_map_resource (in /home/pmacarth/src/spdk/examples/nvme/identify/identify)
==8489==    by 0x12B115: pci_probe_all_drivers.part.0 (in /home/pmacarth/src/spdk/examples/nvme/identify/identify)
==8489==    by 0x12B596: rte_eal_pci_probe (in /home/pmacarth/src/spdk/examples/nvme/identify/identify)
==8489==    by 0x11D5B5: spdk_pci_enumerate (pci.c:147)
==8489==  Address 0x63f362e is 14 bytes inside a block of size 32 free'd
==8489==    at 0x4C2ED5B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8489==    by 0x11E6FB: spdk_free_args (init.c:136)
==8489==    by 0x11EBF5: spdk_env_init (init.c:309)
==8489==    by 0x10D2AA: main (identify.c:976)
==8489==  Block was alloc'd at
==8489==    at 0x4C2DB2F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8489==    by 0x11E7D7: _sprintf_alloc (init.c:76)
==8489==    by 0x11EA78: spdk_build_eal_cmdline (init.c:251)
==8489==    by 0x11EA78: spdk_env_init (init.c:282)
==8489==    by 0x10D2AA: main (identify.c:976)
Quoted 1 line

==8489==

Fix this by using strdup() to create separate memory buffers for these strings. Note that this patch will cause valgrind to report memory leaks of these buffers as there is nowhere to free them. Using static buffers is an option but would make these strings have a fixed maximum length whereas there is currently no limit defined by the API.

[1] http://spdk.io
[2] https://github.com/zrlio/urdma

Fixes af75078fece3 ("first public release")
Cc [email protected]
Signed-off-by Patrick MacArthur <[email protected]>
---
 lib/librte_eal/linuxapp/eal/eal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
lib/librte_eal/linuxapp/eal/eal.c +2 -2
index 48f12f44cfa4..529d2cebc84b 100644
@@ -569,11 +569,11 @@ eal_parse_args(int argc, char **argv)
 			break;
 
 		case OPT_HUGE_DIR_NUM:
-			internal_config.hugepage_dir = optarg;
+			internal_config.hugepage_dir = strdup(optarg);
 			break;
 
 		case OPT_FILE_PREFIX_NUM:
-			internal_config.hugefile_prefix = optarg;
+			internal_config.hugefile_prefix = strdup(optarg);
 			break;
 
 		case OPT_SOCKET_MEM_NUM:
Signature · 1 line
2.11.0
Acked-by on v1 1/1
#1 lore raw
Quoted 79 lines · Patrick MacArthur

Normally, command line argument strings are considered immutable, but SPDK [1] and urdma [2] construct argv arrays to pass to rte_eal_init(). These strings are allocated using malloc() and freed after DPDK initialization with free(). However, in the case of --file-prefix and --huge-dir, DPDK takes the pointer to these strings in argv directly. If a secondary process calls rte_eal_pci_probe() after rte_eal_init() returns, as is done by SPDK, this causes a use-after-free error because the strings have been freed by the calling code immediately after rte_eal_init() returns.

This problem was observed when running SPDK example programs as a secondary process and causes the secondary processes to fail:

Quoted 11 lines

Starting DPDK 16.11.1 initialization...
[ DPDK EAL parameters: identify -c 4 --file-prefix=spdk3260 --base-virtaddr=0x1000000000 --proc-type=auto ]

EAL: Detected 40 lcore(s)
EAL: Auto-detected process type: SECONDARY
EAL: Probing VFIO support...
EAL: VFIO support initialized
EAL: PCI device 0000:81:00.0 on NUMA socket 1
EAL:   probe driver: 8086:953 spdk_nvme
EAL:   cannot connect to primary process!
EAL: Error - exiting with code: 1
Cause: Requested device 0000:81:00.0 cannot be used

Running strace shows that the file prefix has been zero'd out by the time that the secondary process attempts to probe the NVMe device.

The use-after-free errors can be easily detected with valgrind:

Quoted 24 lines

==8489== Invalid read of size 1

==8489==    at 0x4C30D22: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8489==    by 0x58DB955: vfprintf (vfprintf.c:1637)
==8489==    by 0x59A4685: __vsnprintf_chk (vsnprintf_chk.c:63)
==8489==    by 0x59A45E7: __snprintf_chk (snprintf_chk.c:34)
==8489==    by 0x1246AB: get_socket_path.constprop.0 (in /home/pmacarth/src/spdk/examples/nvme/identify/identify)
==8489==    by 0x124B09: vfio_mp_sync_connect_to_primary (in /home/pmacarth/src/spdk/examples/nvme/identify/identify)
==8489==    by 0x123BE4: vfio_get_group_fd.part.1 (in /home/pmacarth/src/spdk/examples/nvme/identify/identify)
==8489==    by 0x124366: vfio_setup_device (in /home/pmacarth/src/spdk/examples/nvme/identify/identify)
==8489==    by 0x126C8A: pci_vfio_map_resource (in /home/pmacarth/src/spdk/examples/nvme/identify/identify)
==8489==    by 0x12B115: pci_probe_all_drivers.part.0 (in /home/pmacarth/src/spdk/examples/nvme/identify/identify)
==8489==    by 0x12B596: rte_eal_pci_probe (in /home/pmacarth/src/spdk/examples/nvme/identify/identify)
==8489==    by 0x11D5B5: spdk_pci_enumerate (pci.c:147)
==8489==  Address 0x63f362e is 14 bytes inside a block of size 32 free'd
==8489==    at 0x4C2ED5B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8489==    by 0x11E6FB: spdk_free_args (init.c:136)
==8489==    by 0x11EBF5: spdk_env_init (init.c:309)
==8489==    by 0x10D2AA: main (identify.c:976)
==8489==  Block was alloc'd at
==8489==    at 0x4C2DB2F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8489==    by 0x11E7D7: _sprintf_alloc (init.c:76)
==8489==    by 0x11EA78: spdk_build_eal_cmdline (init.c:251)
==8489==    by 0x11EA78: spdk_env_init (init.c:282)
==8489==    by 0x10D2AA: main (identify.c:976)
Quoted 1 line

==8489==

Fix this by using strdup() to create separate memory buffers for these strings. Note that this patch will cause valgrind to report memory leaks of these buffers as there is nowhere to free them. Using static buffers is an option but would make these strings have a fixed maximum length whereas there is currently no limit defined by the API.

[1] http://spdk.io
[2] https://github.com/zrlio/urdma

Fixes af75078fece3 ("first public release")
Cc [email protected]
Signed-off-by Patrick MacArthur <[email protected]>
---
  lib/librte_eal/linuxapp/eal/eal.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
lib/librte_eal/linuxapp/eal/eal.c +0 -0
index 48f12f44cfa4..529d2cebc84b 100644
@@ -569,11 +569,11 @@ eal_parse_args(int argc, char **argv)
  			break;
 
  		case OPT_HUGE_DIR_NUM:

lib/librte_eal/linuxapp/eal/eal.c eal_parse_args(int argc, char **argv) · line 579

Quoted 10 lines
lib/librte_eal/linuxapp/eal/eal.c +2 -2
-			internal_config.hugepage_dir = optarg;
+			internal_config.hugepage_dir = strdup(optarg);
  			break;
 
  		case OPT_FILE_PREFIX_NUM:
-			internal_config.hugefile_prefix = optarg;
+			internal_config.hugefile_prefix = strdup(optarg);
  			break;
 
  		case OPT_SOCKET_MEM_NUM:
Acked-by Sergio Gonzalez Monroy <[email protected]>
Applied
#2 lore raw

04/09/2017 12:12, Sergio Gonzalez Monroy:

Quoted 10 lines · Patrick MacArthur
Quoted 9 lines

Normally, command line argument strings are considered immutable, but SPDK [1] and urdma [2] construct argv arrays to pass to rte_eal_init(). These strings are allocated using malloc() and freed after DPDK initialization with free(). However, in the case of --file-prefix and --huge-dir, DPDK takes the pointer to these strings in argv directly. If a secondary process calls rte_eal_pci_probe() after rte_eal_init() returns, as is done by SPDK, this causes a use-after-free error because the strings have been freed by the calling code immediately after rte_eal_init() returns.

[...]

Quoted 9 lines
Quoted 9 lines

Fix this by using strdup() to create separate memory buffers for these strings. Note that this patch will cause valgrind to report memory leaks of these buffers as there is nowhere to free them. Using static buffers is an option but would make these strings have a fixed maximum length whereas there is currently no limit defined by the API.

[1] http://spdk.io
[2] https://github.com/zrlio/urdma

Quoted 6 lines
Quoted 4 lines
Fixes af75078fece3 ("first public release")
Cc [email protected]
Signed-off-by Patrick MacArthur <[email protected]>
Acked-by Sergio Gonzalez Monroy <[email protected]>

Applied, thanks