Re: [PATCH v7 4/7] drm/tyr: add GPU virtual memory (VM) support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Casting 64-bit physical and virtual addresses to `usize` causes silent truncation on 32-bit platforms with LPAE, leading to incorrect mappings and memory corruption.
- [High] Insufficient `GpuVaAlloc` preallocation causes map/unmap operations to fail midway with `EINVAL`, leaving hardware page tables out of sync with the GPUVM tree.
- [High] Unmapping regions backed by non-2MB-aligned physical addresses fails and leaks the mappings because `pt_unmap` incorrectly assumes the mapped page size is 2MB.
- [High] Missing bounds check on GEM object size in `map_bo_range` allows creating overgrown mappings, leading to permanent memory leaks and GPUVM tree corruption.
- [Medium] Successfully mapped pages during a failing `pt.map_pages` call are not unmapped, permanently leaking page table entries.
--

--- Patch [4]: [PATCH v7 4/7] drm/tyr: add GPU virtual memory (VM) support ---
commit 809ab9f421f459ef56a480ccf227ec9341b24878
Author: Boris Brezillon <boris.brezillon@collabora.com>

drm/tyr: add GPU virtual memory (VM) support

Add GPU virtual address space management using the DRM GPUVM framework.
Each virtual memory space is backed by ARM64 LPAE Stage 1 page tables
and can be mapped into hardware address space slots for GPU execution.

> diff --git a/drivers/gpu/drm/tyr/vm.rs b/drivers/gpu/drm/tyr/vm.rs
> index 0000000000000..2358096afc7e8 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/tyr/vm.rs
[ ... ]
>     pub(crate) fn map_bo_range(
>         &self,
>         bo: &Bo,
>         bo_offset: u64,
>         size: u64,
>         va: u64,
>         flags: VmMapFlags,
>     ) -> Result {
>         let req = VmOpRequest {
>             op_type: VmOpType::Map(VmMapArgs {
>                 vm_bo: self.gpuvm.obtain(bo, ())?,
>                 flags,
>                 bo_offset,
>             }),
>             region: va..(va + size),
>         };

[Severity: High]
Does this code allow mapping a region larger than the actual GEM object?

If bo_offset + size exceeds the object's size, sm_step_map() will finish
when the scatter-gather table is exhausted but register a GPUVA node for
the full oversized range. When this oversized virtual address range is
later unmapped, the driver might attempt to unmap page table entries that
were never populated, which can cause pt.unmap_pages() to return 0 and
abort the unmap operation.

>         let mut resources = VmOpResources {
>             preallocated_gpuvas: [
>                 Some(GpuVaAlloc::<GpuVmData<'bound>>::new(GFP_KERNEL)?),
>                 Some(GpuVaAlloc::<GpuVmData<'bound>>::new(GFP_KERNEL)?),
>                 Some(GpuVaAlloc::<GpuVmData<'bound>>::new(GFP_KERNEL)?),
>             ],
>         };

[Severity: High]
Is preallocating 3 VAs sufficient for a map operation? 

If the new mapping overlaps and splits two existing mappings, it can trigger
two remaps. A single remap consumes 2 preallocated VAs, plus 1 for the new
mapping, requiring up to 5 VAs in total. Can this cause the map operation to
fail midway with EINVAL and leave the software GPUVM tracking tree out of
sync with the hardware page tables?

[ ... ]
>     pub(crate) fn unmap_range(&self, va: u64, size: u64) -> Result {
>         let req = VmOpRequest {
>             op_type: VmOpType::Unmap,
>             region: va..(va + size),
>         };
>         let mut resources = VmOpResources {
>             preallocated_gpuvas: [
>                 Some(GpuVaAlloc::<GpuVmData<'bound>>::new(GFP_KERNEL)?),
>                 Some(GpuVaAlloc::<GpuVmData<'bound>>::new(GFP_KERNEL)?),
>                 None,
>             ],
>         };

[Severity: High]
Will this array have enough VAs for a complex unmap? 

An unmap operation that splits two existing mappings could require up to 4 VAs
for the leftover segments, but this initialization only allocates 2.

[ ... ]
> fn pt_map(
>     pt: &IoPageTable<'_, ARM64LPAES1>,
>     iova: u64,
>     paddr: u64,
>     len: u64,
>     prot: u32,
> ) -> Result<u64> {
[ ... ]
>         let (mapped, result) = unsafe {
>             pt.map_pages(
>                 curr_iova as usize,
>                 (curr_paddr as usize).try_into().unwrap(),
>                 pgsize as usize,
>                 pgcount as usize,
>                 prot,
>                 GFP_KERNEL,
>             )
>         };

[Severity: Critical]
Does casting curr_paddr and curr_iova to usize truncate 64-bit addresses
on 32-bit platforms with LPAE? 

This could cause incorrect physical pages to be mapped into the GPU's
address space, potentially leading to unauthorized memory access or data
corruption.

>         if let Err(e) = result {
>             pr_err!("pt.map_pages failed at iova {:#x}: {:?}\n", curr_iova, e);
>             if segment_mapped > 0 {
>                 let _ = pt_unmap(pt, iova..(iova + segment_mapped));
>             }
>             return Err(e);
>         }

[Severity: Medium]
If pt.map_pages() succeeds in mapping some pages but returns an error for
a subsequent page, does this error path leak the pages that were just
successfully mapped? 

The cleanup unmaps segment_mapped (the bytes mapped in previous loop
iterations) but doesn't appear to include the mapped bytes returned by
the failing pt.map_pages() call.

[ ... ]
> fn pt_unmap(pt: &IoPageTable<'_, ARM64LPAES1>, range: Range<u64>) -> Result {
>     let mut iova = range.start;
>     let mut bytes_left_to_unmap = range.end - range.start;
> 
>     while bytes_left_to_unmap > 0 {
>         let (pgsize, pgcount) = get_pgsize(iova, bytes_left_to_unmap);
> 
>         let unmapped = unsafe { pt.unmap_pages(iova as usize, pgsize as usize, pgcount as usize) };

[Severity: High]
Can this fail to unmap regions if they were originally backed by physical
addresses that were not 2MB-aligned?

In pt_map(), the page size is calculated using both virtual and physical
addresses:

    get_pgsize(curr_iova | curr_paddr, remaining);

However, pt_unmap() calculates the page size using only the virtual address:

    get_pgsize(iova, bytes_left_to_unmap);

If a user maps a physical address that is not 2MB-aligned into a virtual
address that is 2MB-aligned, it appears pt_map() will fall back to 4KB
pages, but pt_unmap() will attempt to unmap using 2MB blocks.

[Severity: Critical]
Does casting iova to usize here also cause silent truncation on 32-bit
platforms?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-fw-boot-b4-v7-0-afd8dc1494dd@collabora.com?part=4