Re: [PATCH] drm/ttm: clamp the prefault window to the buffer object

["Christian König" <christian.koenig@amd.com>]

On 8/6/26 05:43, Baul Lee wrote:
> ttm_bo_vm_fault_reserved() derives two page indices from the caller's
> mmap(2) arguments and bounds only one of them:
> 
>         page_offset = ((address - vma->vm_start) >> PAGE_SHIFT) +
>                 vma->vm_pgoff - drm_vma_node_start(&bo->base.vma_node);
>         page_last = vma_pages(vma) + vma->vm_pgoff -
>                 drm_vma_node_start(&bo->base.vma_node);
> 
>         if (unlikely(page_offset >= PFN_UP(bo->base.size)))
>                 return VM_FAULT_SIGBUS;
> 
> bo->base.size appears once in the function, bounding page_offset on
> entry.  page_last comes straight from vma_pages(vma) and is the loop
> terminator:
> 
>         if (unlikely(++page_offset >= page_last))
>                 break;
> 
> so the object size never bounds it.  For an object of N pages, a fault
> on the last in-object page passes the entry test with page_offset
> N - 1, and the prefault loop then walks N..N+14, reading
> ttm->pages[page_offset] or
> ttm_bo_io_mem_pfn(bo, page_offset) and installing each frame with
> vmf_insert_pfn_prot().
> 
> page_last exceeds the object whenever the VMA is longer than it.
> drm_gem_mmap_obj() rejects that on the DRM node, but the fbdev path
> reaches the object function through drm_gem_prime_mmap(), which does
> not.  It is also exceeded by a mapping no longer than the object taken
> at a nonzero file offset, so the handler needs its own bound.
> 
> With a 128-page object mapped 192 pages long,

And exactly that sentence explains why this whole patch is superfluous.

A 128 page object absolutely *can't* be mapped into 192 pages VMA!

If we would allow that the driver side could crash extremely badly long before we even get here.

Do you have any reproducer which exercises this?

Regards,
Christian.



> one read fault at index
> N - 1 leaves the fifteen frames after the object readable through the
> mapping; on a fresh mapping, reading index N without first faulting
> N - 1 is SIGBUS.  For a system-memory placement the page array is
> over-read as well:
> 
>   BUG: KASAN: slab-out-of-bounds in ttm_bo_vm_fault_reserved+0x248/0x57c
>   Read of size 8 at addr ffff0000078aac00 by task e1/219
>    __asan_load8+0x84/0xb0
>    ttm_bo_vm_fault_reserved+0x248/0x57c
>    ttm_bo_vm_fault+0xe4/0x140
>    __do_fault+0x6c/0x2f0
> 
> Clamp page_last to the object.
> 
> Discovered by XBOW, triaged by Baul Lee <baul.lee@xbow.com>
> 
> Fixes: ba4e7d973dd0 ("drm: Add the TTM GPU memory manager subsystem.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Baul Lee <baul.lee@xbow.com>
> ---
>  drivers/gpu/drm/ttm/ttm_bo_vm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> index a80510489c45..14ebf6ee3c47 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> @@ -212,6 +212,7 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
>                 vma->vm_pgoff - drm_vma_node_start(&bo->base.vma_node);
>         page_last = vma_pages(vma) + vma->vm_pgoff -
>                 drm_vma_node_start(&bo->base.vma_node);
> +       page_last = min_t(unsigned long, page_last, PFN_UP(bo->base.size));
> 
>         if (unlikely(page_offset >= PFN_UP(bo->base.size)))
>                 return VM_FAULT_SIGBUS;
> --
> 2.50.1 (Apple Git-155)