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 <[email protected]> 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/[email protected]?part=1
