Re: [PATCH v2 4/4] vfio: selftests: Allow a size for vfio_dma_mapping_perf_test

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Using wordexp() on argv[0] without quoting causes incorrect argument splitting, and using wordexp() on test arguments allows unintended shell expansion.
- [Low] The diagnostic print statement for the mapped region size statically calculates the size in Gigabytes, truncating custom sizes smaller than 1GB to 0G.
--

--- Patch [4]: [PATCH v2 4/4] vfio: selftests: Allow a size for vfio_dma_mapping_perf_test ---
commit cff64ba01747807f5a7be83ede29a96ad3859dc3
Author: Aaron Lewis <[email protected]>

vfio: selftests: Allow a size for vfio_dma_mapping_perf_test

This commit allows the user to specify a custom DMA region size via the
command line for vfio_dma_mapping_perf_test. It also introduces a way to
pass arguments through to the test harness using a new '-a' option.

> diff --git a/tools/testing/selftests/vfio/vfio_dma_mapping_perf_test.c b/tools/testing/selftests/vfio/vfio_dma_mapping_perf_test.c
> index 5ef85deba4ee0..af2273a0c6f52 100644
> --- a/tools/testing/selftests/vfio/vfio_dma_mapping_perf_test.c
> +++ b/tools/testing/selftests/vfio/vfio_dma_mapping_perf_test.c

[ ... ]

> @@ -159,17 +165,17 @@ static void teardown_memfd(int fd, u64 size, void *vaddr)
>  
>  TEST_F(vfio_dma_mapping_perf_memfd_test, dma_map_unmap_from_file)
>  {
> -	const u64 size = SZ_1G;
> -	const int flags = variant->mmap_flags;
> +	const u64 size = test_params.size;
> +	const int mmap_flags = variant->mmap_flags;
>  	struct dma_region region;
>  	int fd;
>  
>  	printf("mmap size = %lluG\n", (unsigned long long)(size / SZ_1G));

[Severity: Low]
Does this print statement output "0G" when a size smaller than 1GB is
provided? It looks like the division by SZ_1G truncates custom sizes like
512M to zero.

> -	region.vaddr = setup_memfd(&fd, size, variant->mmap_flags, variant->memfd_flags);
> +	region.vaddr = setup_memfd(&fd, size, mmap_flags, variant->memfd_flags);
>  
>  	/* Skip the test if there aren't enough HugeTLB pages available. */

[ ... ]

> @@ -185,8 +191,141 @@ TEST_F(vfio_dma_mapping_perf_memfd_test, dma_map_unmap_from_file)

[ ... ]

> +static void populate_harness_args(struct harness_args *args, const char *argv_0,
> +				  const char *cmdlne)
> +{
> +	int flags = WRDE_NOCMD;
> +
> +	if (!args->argv) {
> +		/*
> +		 * Initialize the argument list with the program name (argv[0]).
> +		 * WRDE_NOCMD disables command substitution for safety.
> +		 */
> +		if (wordexp(argv_0, &args->exp, flags) != 0)

[Severity: Medium]
Will this wordexp() call split the test executable's path into multiple
arguments if the path contains spaces? If a user runs this from a directory
like '/opt/my tests/test', it seems it would corrupt the test harness
argument list and cause test_harness_run() to fail.

> +			VFIO_FAIL("Failed to evaluate test harness argv_0 args!");
> +	}
> +
> +	flags |= WRDE_APPEND;
> +
> +	/*
> +	 * Use wordexp() to reliably parse the user-supplied command line string
> +	 * into individual arguments, respecting shell quoting and escaping rules.
> +	 * WRDE_APPEND merges these new arguments with the earlier argv[0].
> +	 */
> +	if (wordexp(cmdlne, &args->exp, flags) != 0)

[Severity: Medium]
Does using wordexp() on the user-provided arguments allow unintended shell
globbing? For instance, if a user passes an argument like '-a "-v test*"',
would wordexp() expand this against local files if a match exists, rather
than passing the literal wildcard to the test harness filter?

> +		VFIO_FAIL("Failed to evaluate test harness cmdlne args!");
> +
> +	args->argc = args->exp.we_wordc;
> +	args->argv = args->exp.we_wordv;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4