From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sergio Gonzalez Monroy Subject: Re: [PATCH] eal: Copy raw strings taken from command line Date: Mon, 4 Sep 2017 11:12:17 +0100 Message-ID: References: <20170804185357.6612-1-patrick@patrickmacarthur.net> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Cc: stable@dpdk.org To: Patrick MacArthur , dev@dpdk.org Return-path: In-Reply-To: <20170804185357.6612-1-patrick@patrickmacarthur.net> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 04/08/2017 19:53, Patrick MacArthur wrote: > 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: > >> 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: > >> ==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) >> ==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: stable@dpdk.org > > Signed-off-by: Patrick MacArthur > --- > lib/librte_eal/linuxapp/eal/eal.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c > index 48f12f44cfa4..529d2cebc84b 100644 > --- a/lib/librte_eal/linuxapp/eal/eal.c > +++ b/lib/librte_eal/linuxapp/eal/eal.c > @@ -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: Acked-by: Sergio Gonzalez Monroy