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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The -h option and invalid arguments fail to exit the program, causing unintended execution of all tests.
- [Medium] Multiple usages of the '-a' flag overwrite the wordexp structure without WRDE_REUSE, causing memory leaks and dropping previous arguments.
- [Medium] Using size_t for math and return type in parse_size() causes undefined behavior and silent truncation on 32-bit architectures.
--

commit d938d0d26cc3c95a10e15ae011fce57f55a320c9
Author: Aaron Lewis <aaronlewis@google.com>

vfio: selftests: Allow a size for vfio_dma_mapping_perf_test
    
Allow the user to specify a DMA region size via the command line for
vfio_dma_mapping_perf_test.

> 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 4639bda7ebaa3..2d8fc3e8e0787 100644
> --- a/tools/testing/selftests/vfio/vfio_dma_mapping_perf_test.c
> +++ b/tools/testing/selftests/vfio/vfio_dma_mapping_perf_test.c
[ ... ]
> +size_t parse_size(const char *size)
> +{
> +	size_t base;
> +	char *scale;
> +	int shift = 0;
[ ... ]
> +	switch (tolower(*scale)) {
> +	case 't':
> +		shift = 40;
> +		break;
[ ... ]
> +	VFIO_ASSERT_TRUE((base << shift) >> shift == base,
> +			 "Overflow scaling size!");
> +
> +	return base << shift;
> +}

[Severity: Medium]
On 32-bit architectures where size_t is 32-bit, will shifting a 32-bit integer 
by 40 when parsing terabytes cause undefined behavior?

Because compilers can mask over-sized shift counts, the overflow check here
might evaluate as true and silently pass while returning a drastically
truncated size. Should this function use u64 internally and as its return
type to match test_params.size?

[ ... ]
> +static void populate_harness_args(struct harness_args *args, const char *argv_0,
> +				  const char *cmdlne)
> +{
> +	if (wordexp(argv_0, &args->exp, WRDE_NOCMD) == 0 &&
> +	    wordexp(cmdlne, &args->exp, WRDE_APPEND | WRDE_NOCMD) == 0) {

[Severity: Medium]
If a user passes multiple '-a' flags, does the first wordexp() call without
WRDE_REUSE or WRDE_APPEND leak the previously allocated memory in args->exp?

This also looks like it would discard any arguments appended by earlier '-a'
flags, and potentially leave args->argv pointing to leaked allocations if the
second wordexp() fails.

> +		args->argc = args->exp.we_wordc;
> +		args->argv = args->exp.we_wordv;
> +	}
> +}
> +
> +static void setup_test(struct harness_args *args, int argc, char *argv[])
> +{
> +	int opt;
> +
> +	test_params = (struct test_params) {
> +		.size = SZ_1G,
> +	};
> +
> +	while ((opt = getopt(argc, argv, "a:b:h")) != -1) {
> +		switch (opt) {
[ ... ]
> +		case 'h':
> +		default:
> +			help(argv[0]);
> +			goto out;
> +		}
> +	}
> +
> +out:
> +	// Reset getopt() state to allow the test harness to use it.
> +	optind = 1;
> +}

[Severity: Medium]
When the '-h' option or an invalid argument is provided, does jumping to 'out' 
unintentionally allow the program to continue executing?

Because setup_test() returns normally to main() in this path, main() will
subsequently execute test_harness_run(), which runs the full test suite
immediately after printing the help message instead of terminating.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701203311.326798-1-aaronlewis@google.com?part=3