Re: [dpdk-dev] [PATCH] eal: Copy raw strings taken from command line

[Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>]

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 <patrick@patrickmacarthur.net>
> ---
>   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 <sergio.gonzalez.monroy@intel.com>