Re: [PATCH] drm/i915/selftests: Fix unlocked mm.stolen in reset selftest

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Medium] The logic to determine if a stolen page is unused evaluates to always false, neutralizing the selftest.
- [Medium] Missing lock protection when accessing the globally shared `ggtt->error_capture` memory, risking arbitrary memory corruption.
- [Medium] Time-of-Check to Time-of-Use (TOCTOU) race condition between checking page availability and poisoning it.
--

--- Patch [1]: [PATCH] drm/i915/selftests: Fix unlocked mm.stolen in reset selftest ---
commit eab33f79c43e7ed3d420674fa2faede0152e1b0b
Author: Sebastian Brzezinka <[email protected]>

drm/i915/selftests: Fix unlocked mm.stolen in reset selftest

This commit fixes an unlocked access to i915->mm.stolen in the reset selftest
by wrapping __drm_mm_interval_first() calls with i915->mm.stolen_lock. This
prevents a NULL pointer dereference observed during igt_reset_device_stolen().

Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/16841

> diff --git a/drivers/gpu/drm/i915/gt/selftest_reset.c b/drivers/gpu/drm/i915/gt/selftest_reset.c
> index 2cfc23c58e909..eb6b2261329da 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_reset.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_reset.c

[ ... ]

> @@ -95,9 +96,12 @@ __igt_reset_stolen(struct intel_gt *gt,
>  		dma_addr_t dma = (dma_addr_t)dsm->start + (page << PAGE_SHIFT);
>  		void __iomem *s;
>  		void *in;
>  
>  		ggtt->vm.insert_page(&ggtt->vm, dma,
>  				     ggtt->error_capture.start,
>  				     i915_gem_get_pat_index(gt->i915,
>  							    I915_CACHE_NONE),
>  				     0);

[Severity: Medium]
This is a pre-existing issue, but does this code modify the globally shared
ggtt->error_capture memory without serialization?

If a real GPU error occurs or sysfs is accessed concurrently during test
execution, could the error capture PTE be overwritten, risking arbitrary
memory corruption?

>  		mb();
>  
>  		s = io_mapping_map_wc(&ggtt->iomap,
>  				      ggtt->error_capture.start,
>  				      PAGE_SIZE);
>  
> -		if (!__drm_mm_interval_first(&gt->i915->mm.stolen,
> -					     page << PAGE_SHIFT,
> -					     ((page + 1) << PAGE_SHIFT) - 1))
> +		mutex_lock(&gt->i915->mm.stolen_lock);
> +		unused = !__drm_mm_interval_first(&gt->i915->mm.stolen,
> +						  page << PAGE_SHIFT,
> +						  ((page + 1) << PAGE_SHIFT) - 1);

[Severity: Medium]
This is a pre-existing issue, but does this logic to determine if a stolen
page is unused evaluate to always false?

Looking at __drm_mm_interval_first(), it returns &mm->head_node when no
interval is found, which is a non-NULL pointer. This means
!__drm_mm_interval_first(...) is always false.

Does this neutralize the selftest by preventing it from verifying or
poisoning unused stolen memory pages?

> +		mutex_unlock(&gt->i915->mm.stolen_lock);
> +		if (unused)
>  			memset_io(s, STACK_MAGIC, PAGE_SIZE);

[Severity: Medium]
This is a pre-existing issue, but is there a time-of-check to time-of-use
(TOCTOU) race condition here?

The lock gt->i915->mm.stolen_lock is released immediately after checking
the page availability, and before memset_io() poisons it.

Could a concurrent thread allocate this page before memset_io() writes
STACK_MAGIC to it, leading to unintended memory corruption of legitimate
stolen memory allocations?

>  
>  		in = (void __force *)s;

[ ... ]

> @@ -143,10 +147,12 @@ __igt_reset_stolen(struct intel_gt *gt,
>  			in = tmp;
>  		x = crc32_le(0, in, PAGE_SIZE);
>  
> -		if (x != crc[page] &&
> -		    !__drm_mm_interval_first(&gt->i915->mm.stolen,
> -					     page << PAGE_SHIFT,
> -					     ((page + 1) << PAGE_SHIFT) - 1)) {
> +		mutex_lock(&gt->i915->mm.stolen_lock);
> +		unused = !__drm_mm_interval_first(&gt->i915->mm.stolen,
> +						  page << PAGE_SHIFT,
> +						  ((page + 1) << PAGE_SHIFT) - 1);
> +		mutex_unlock(&gt->i915->mm.stolen_lock);
> +		if (x != crc[page] && unused) {

[Severity: Medium]
This is a pre-existing issue, but this check for clobbered unused pages
appears to also be neutralized by the always-false condition described
above since unused will always be false.

>  			pr_debug("unused stolen page %pa modified by GPU reset\n",
>  				 &page);
>  			if (count++ == 0)

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