Re: [PATCH 4/4] drm/imagination: Add sparse mappings to VM_BIND

[sashiko-bot@kernel.org]

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 <gye976@gmail.com>
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/20260817-pvr-vm-bind-v1-0-0a0f21be7d38@gmail.com?part=4