Re: [PATCH v5 4/4] selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing `errno` preservation leads to potential clobbering before evaluation.
- [Medium] Unconditional unmapping of `req` causes a file descriptor leak if `ioctl` unexpectedly succeeds.
--

--- Patch [4]: [PATCH v5 4/4] selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test ---
commit 43e605bb1fee2426e027813c0311f1910f7ea15e
Author: Baineng Shou <shoubaineng@gmail.com>

selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test

Add a test case that verifies no file descriptor is leaked when
DMA_HEAP_IOCTL_ALLOC succeeds internally but copy_to_user() fails
to deliver the fd number back to userspace.

> diff --git a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
> index fc9694fc4e89e..bd58e5b06c8b6 100644
> --- a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
> +++ b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c

[ ... ]

> +static void test_alloc_no_fd_leak_on_efault(char *heap_name)
> +{

[ ... ]

> +	/*
> +	 * Make the page read-only: copy_from_user() in the kernel will
> +	 * still succeed (it already ran), but copy_to_user() that writes
> +	 * the fd number back will fault.
> +	 */
> +	mprotect(req, page_size, PROT_READ);
> +
> +	ret = ioctl(heap_fd, DMA_HEAP_IOCTL_ALLOC, req);
> +
> +	/* Re-allow writes so munmap can clean up */
> +	mprotect(req, page_size, PROT_READ | PROT_WRITE);
> +	munmap(req, page_size);
> +
> +	if (ret != -1 || errno != EFAULT) {

[Severity: Medium]
Will this evaluate the correct errno from the ioctl call?

The errno variable is checked after executing mprotect and munmap. If either of
these functions modify errno (for example, if they fail), the test will
evaluate the wrong errno and incorrectly skip with a misleading message instead
of reporting a failure or success properly.

[Severity: Medium]
Also, could this leak the newly allocated file descriptor in the test process
if the ioctl call unexpectedly succeeds?

If the ioctl call successfully allocates a file descriptor (for instance, if
the initial mprotect fails to make the page read-only or due to a kernel bug),
the new file descriptor is written to req->fd.

Since the test unconditionally calls munmap(req) immediately after, it destroys
the only reference to the allocated file descriptor. The test then detects
unexpected success, skips, and leaks the file descriptor in the test process.

> +		/*
> +		 * If the ioctl didn't fail with EFAULT, either the kernel
> +		 * handled it differently or mprotect raced.  Skip rather
> +		 * than giving a false pass/fail.
> +		 */
> +		ksft_test_result_skip(
> +			"ioctl did not return EFAULT (ret=%d errno=%d), skipping\n",
> +			ret, errno);
> +		goto out;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730062645.233148-1-shoubaineng@gmail.com?part=4