Re: [PATCH] drm/amdgpu: reject mapping info for unmapped BOs

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Duplicate `if (!bo_va)` check introduces redundant logic.

Pre-existing issues:
- [High] Deadlock risk due to GFP_KERNEL allocation with dma_resv locks held.
- [High] Denial of Service risk via unverified large kernel memory allocation.
--

commit 3feae89c84a776461a942a018d277330e825a587
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>

drm/amdgpu: reject mapping info for unmapped BOs

AMDGPU_GEM_OP_GET_MAPPING_INFO looks up the BO's VM mapping and then
iterates the valid and invalid mapping lists unconditionally. A GEM BO can
be queried before it has been mapped into the file VM, in which case
amdgpu_vm_bo_find() returns NULL and the list walk dereferences it.

Return -ENOENT for an unmapped BO, matching the VA operation path that
already rejects missing BO-VA state before touching the mapping lists.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 28f89ae8ca745..4ef08737390a2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -1087,6 +1087,12 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev, void *data,
>  		struct drm_amdgpu_gem_vm_entry *vm_entries;
>  		struct amdgpu_bo_va_mapping *mapping;
>  		int num_mappings = 0;
> +
> +		if (!bo_va) {
> +			r = -ENOENT;
> +			goto out_exec;
> +		}
> +

[Severity: Low]
Does this introduce a redundant check? There appears to be an identical check
immediately following the comment block in amdgpu_gem_op_ioctl():

		/*
		 * num_entries is set as an input to the size of the user-allocated array of
		 * drm_amdgpu_gem_vm_entry stored at args->value.
		 * num_entries is sent back as output as the number of mappings the bo has.
		 * If that number is larger than the size of the array, the ioctl must
		 * be retried.
		 */
		if (!bo_va) {
			r = -ENOENT;
			goto out_exec;
		}

>  		/*
>  		 * num_entries is set as an input to the size of the user-allocated array of
>  		 * drm_amdgpu_gem_vm_entry stored at args->value.

[Severity: High]
This is a pre-existing issue, but does this allocation risk a deadlock when
memory pressure occurs?

amdgpu_gem_op_ioctl() {
	...
	vm_entries = kvcalloc(args->num_entries, sizeof(*vm_entries), GFP_KERNEL);
	...
}

Using GFP_KERNEL here allows direct memory reclaim, which can invoke the DRM
shrinker. If the shrinker attempts to acquire dma_resv locks to evict buffer
objects, it could cause a circular locking dependency (ABBA deadlock) since
gobj's dma_resv is already locked earlier in this path via drm_exec_lock_obj().

[Severity: High]
This is another pre-existing issue, but does this same allocation allow for an
unbounded memory allocation that could lead to a denial of service?

amdgpu_gem_op_ioctl() {
	...
	if (args->num_entries > INT_MAX / sizeof(*vm_entries)) {
		r = -EINVAL;
		goto out_exec;
	}

	vm_entries = kvcalloc(args->num_entries, sizeof(*vm_entries), GFP_KERNEL);
	...
}

An unprivileged user could request an allocation size up to INT_MAX (~2GB)
because the kernel does not verify the actual number of mappings before
allocating the vm_entries array based solely on the user-supplied
args->num_entries.

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