drm/ttm: clamp the prefault window to the buffer object

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

Posted by Baul Lee 1 month, 3 weeks ago

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, 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)

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

Posted by Christian König 1 month, 3 weeks ago

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)

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

Posted by Matthew Brost 1 month, 3 weeks ago

On Thu, Aug 06, 2026 at 12:43:56PM +0900, 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, 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>

You are going to want an Assisted-by tag here as XBOW is an AI tool?

> 
> Fixes: ba4e7d973dd0 ("drm: Add the TTM GPU memory manager subsystem.")

This won't apply to ba4e7d973dd0. More below.

> 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));

This looks correct but maybe to make backporting easier all the way to
ba4e7d973dd0, we do this instead...

diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
index a80510489c45..3529371a37d5 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
@@ -274,7 +274,8 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
                }

                address += PAGE_SIZE;
-               if (unlikely(++page_offset >= page_last))
+               if (unlikely(++page_offset >= page_last ||
+                            page_offset >= PFN_UP(bo->base.size)))
                        break;
        }
        return ret;

The above code git blame show this line was last modified in
ba4e7d973dd0.

Then in non-fixes patch, do it like you have it here.

Matt

>  
>  	if (unlikely(page_offset >= PFN_UP(bo->base.size)))
>  		return VM_FAULT_SIGBUS;
> -- 
> 2.50.1 (Apple Git-155)