Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [Critical] Integer underflow in page table preallocation size calculation allows unprivileged users to cause OOM panic. - [High] Page table preallocation calculates requirements using object offset instead of device address, leading to -ENOMEM during valid mappings. - [High] Mapping errors are masked during scatterlist processing, leaving holes in hardware page tables. -- --- Patch [4]: [PATCH 4/4] drm/imagination: Add sparse mappings to VM_BIND --- commit 8758f312b45a84c218f5a0a8c5687d9c21928110 Author: Gyeyoung Baek <[email protected]> Subject: drm/imagination: Add sparse mappings to VM_BIND This commit adds support for Vulkan sparse residency by allowing ranges to be mapped without naming a buffer object. It introduces DRM_PVR_VM_BIND_OP_MAP_SPARSE, where every page table entry in the range points to the same file-specific dummy page. > diff --git a/drivers/gpu/drm/imagination/pvr_mmu.c > b/drivers/gpu/drm/imagination/pvr_mmu.c > index 3cac482e10347..fc1347d6f233b 100644 > --- a/drivers/gpu/drm/imagination/pvr_mmu.c > +++ b/drivers/gpu/drm/imagination/pvr_mmu.c > @@ -2558,6 +2558,77 @@ pvr_mmu_map_sgl(struct pvr_mmu_op_context *op_ctx, > struct scatterlist *sgl, > return err; > } [Severity: High] This is a pre-existing issue, but does pvr_mmu_map_sgl() mask the original error code on failure? If an error occurs during page table entry creation in pvr_mmu_map_sgl(), the code jumps to the err_destroy_pages label: drivers/gpu/drm/imagination/pvr_mmu.c:pvr_mmu_map_sgl() { ... err_destroy_pages: memcpy(&op_ctx->curr_page, &ptr_copy, sizeof(op_ctx->curr_page)); err = pvr_mmu_op_context_unmap_curr_page(op_ctx, page); return err; } Since pvr_mmu_op_context_unmap_curr_page() typically succeeds and returns 0, the original error code is overwritten with 0. The function then incorrectly signals success to the caller, leaving holes in the hardware page tables. This could cause unpredictable hardware page faults when accessed by the GPU. [ ... ] > diff --git a/drivers/gpu/drm/imagination/pvr_vm.c > b/drivers/gpu/drm/imagination/pvr_vm.c > index f92bfeacd7ac4..d11e192c7726d 100644 > --- a/drivers/gpu/drm/imagination/pvr_vm.c > +++ b/drivers/gpu/drm/imagination/pvr_vm.c > @@ -267,7 +273,7 @@ static int > pvr_vm_bind_op_map_init(struct pvr_vm_bind_op *bind_op, > struct pvr_vm_context *vm_ctx, > struct pvr_gem_object *pvr_obj, u64 offset, > - u64 device_addr, u64 size) > + u64 device_addr, u64 size, bool sparse) > { > struct drm_gem_object *obj = gem_from_pvr_gem(pvr_obj); > const bool is_user = vm_ctx != vm_ctx->pvr_dev->kernel_vm_ctx; > @@ -285,11 +291,19 @@ pvr_vm_bind_op_map_init(struct pvr_vm_bind_op *bind_op, > } > > if (!pvr_device_addr_and_size_are_valid(vm_ctx, device_addr, size) || > - offset & ~PAGE_MASK || size & ~PAGE_MASK || > - offset >= pvr_obj_size || offset_plus_size > pvr_obj_size) > + offset & ~PAGE_MASK || size & ~PAGE_MASK) > + return -EINVAL; [Severity: High] This isn't a bug introduced by this patch, but does the page table preallocation calculate requirements using the object offset instead of the device address? When a VM_BIND request is made, pvr_vm_bind_op_map_init() calls pvr_mmu_op_context_create(), passing offset as the sgt_offset: drivers/gpu/drm/imagination/pvr_mmu.c:pvr_mmu_op_context_create() { ... const u32 l0_start_idx = pvr_page_table_l1_idx(sgt_offset); const u32 l0_end_idx = pvr_page_table_l1_idx(sgt_offset + size); const u32 l0_count = l0_end_idx - l0_start_idx + 1; ... } If the device_addr alignment causes the mapping to span more 2MB or 1GB boundaries than sgt_offset does, this will under-allocate page tables. For instance, mapping an 8KB chunk with offset=0 to a device_addr that crosses a 2MB boundary requires 2 L0 page tables, but the logic using sgt_offset calculates only 1. When the driver attempts to insert the second page, the preallocated pool is empty and returns -ENOMEM. This would cause valid mappings to fail, and for the newly introduced sparse mappings where offset is always 0, it guarantees this failure whenever a sparse mapping crosses a 2MB boundary. [Severity: Critical] This is a pre-existing issue related to the above calculation, but can this integer underflow allow unprivileged users to cause an OOM panic? In the same calculation: drivers/gpu/drm/imagination/pvr_mmu.c:pvr_mmu_op_context_create() { ... const u32 l0_start_idx = pvr_page_table_l1_idx(sgt_offset); const u32 l0_end_idx = pvr_page_table_l1_idx(sgt_offset + size); const u32 l0_count = l0_end_idx - l0_start_idx + 1; ... } The function pvr_page_table_l1_idx() masks out higher bits to return a bounded 9-bit local index: drivers/gpu/drm/imagination/pvr_mmu.c:pvr_page_table_l1_idx() { return (device_addr & ~ROGUE_MMUCTRL_VADDR_PD_INDEX_CLRMSK) >> ROGUE_MMUCTRL_VADDR_PD_INDEX_SHIFT; } If an unprivileged user calls the VM_BIND ioctl with an offset and size such that offset + size crosses a 1GB page directory boundary, l0_end_idx will be smaller than l0_start_idx (e.g., 0 < 511). This causes l0_count to underflow to roughly 4.29 billion, leading to an immediate kernel out-of-memory (OOM) panic as the driver attempts to allocate billions of page table structures in a tight loop. > + /* > + * A sparse mapping repeats one page over a deliberately wider range, > + * so the containment check only applies to ordinary mappings. > + */ > + if (!sparse && > + (offset >= pvr_obj_size || offset_plus_size > pvr_obj_size)) > return -EINVAL; > > bind_op->type = PVR_VM_BIND_TYPE_MAP; > + bind_op->sparse = sparse; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
