[PATCH] drm/gma500: bound the fbdev fault loop to the framebuffer

[PATCH] drm/gma500: bound the fbdev fault loop to the framebuffer

[Baul Lee]

psb_fbdev_vm_fault() maps as many frames as the caller asked for:

	unsigned long pfn = info->fix.smem_start >> PAGE_SHIFT;
	unsigned long page_num = vma_pages(vma);

	for (i = 0; i < page_num; ++i) {
		err = vmf_insert_mixed(vma, address, pfn);
		...
		address += PAGE_SIZE;
		++pfn;
	}

The trip count is the VMA length and the pfn walks up from the
framebuffer base.  info->fix.smem_len, the extent of the framebuffer, is
not read anywhere in the handler, so a mapping longer than the
framebuffer installs present PTEs for the physical frames above it.

Nothing else on the path bounds the length.  psb_fbdev_fb_mmap() only
requires vm_pgoff to be zero, and fb_mmap() passes the length through.

On x86_64 an unprivileged process in group video mapped 4800 pages of a
600-page framebuffer; every page was present and writable, and the 4200
past the object were the frames above it in physical memory.  Nothing is
logged: the kernel installs valid PTEs and every access through them
comes from userspace.

Reject a fault whose page offset is already past the framebuffer and
clamp the loop to the frames the framebuffer owns.

Discovered by XBOW, triaged by Baul Lee <baul.lee@xbow.com>

Fixes: 0867b42113ec ("staging: gma500: Intel GMA500 staging driver")
Cc: stable@vger.kernel.org
Signed-off-by: Baul Lee <baul.lee@xbow.com>
---
 drivers/gpu/drm/gma500/fbdev.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/gma500/fbdev.c b/drivers/gpu/drm/gma500/fbdev.c
index d1e93588234f..393a939a0c37 100644
--- a/drivers/gpu/drm/gma500/fbdev.c
+++ b/drivers/gpu/drm/gma500/fbdev.c
@@ -26,9 +26,15 @@ static vm_fault_t psb_fbdev_vm_fault(struct vm_fault *vmf)
 	unsigned long address = vmf->address - (vmf->pgoff << PAGE_SHIFT);
 	unsigned long pfn = info->fix.smem_start >> PAGE_SHIFT;
 	vm_fault_t err = VM_FAULT_SIGBUS;
+	unsigned long obj_pages = info->fix.smem_len >> PAGE_SHIFT;
 	unsigned long page_num = vma_pages(vma);
 	unsigned long i;
 
+	if (vmf->pgoff >= obj_pages)
+		return VM_FAULT_SIGBUS;
+	if (page_num > obj_pages)
+		page_num = obj_pages;
+
 	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 
 	for (i = 0; i < page_num; ++i) {
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] drm/gma500: bound the fbdev fault loop to the framebuffer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Modifying `vma->vm_page_prot` within the page fault handler violates mm locking rules and creates a data race.
--

--- Patch [1]: [PATCH] drm/gma500: bound the fbdev fault loop to the framebuffer ---
commit e85cf59eaca6225d308fd9d0f4608078a7b0c51e
Author: Baul Lee <baul.lee@xbow.com>

drm/gma500: bound the fbdev fault loop to the framebuffer

This commit limits the number of pages mapped in the page fault handler
to the actual size of the framebuffer object. It rejects faults that start
past the end of the object and clamps requested page counts, preventing
userspace from mapping physical frames beyond the framebuffer boundary.

> diff --git a/drivers/gpu/drm/gma500/fbdev.c b/drivers/gpu/drm/gma500/fbdev.c
> index d1e93588234f5..393a939a0c376 100644
> --- a/drivers/gpu/drm/gma500/fbdev.c
> +++ b/drivers/gpu/drm/gma500/fbdev.c
> @@ -26,9 +26,15 @@ static vm_fault_t psb_fbdev_vm_fault(struct vm_fault *vmf)
>  	unsigned long address = vmf->address - (vmf->pgoff << PAGE_SHIFT);
>  	unsigned long pfn = info->fix.smem_start >> PAGE_SHIFT;
>  	vm_fault_t err = VM_FAULT_SIGBUS;
> +	unsigned long obj_pages = info->fix.smem_len >> PAGE_SHIFT;
>  	unsigned long page_num = vma_pages(vma);
>  	unsigned long i;
>  
> +	if (vmf->pgoff >= obj_pages)
> +		return VM_FAULT_SIGBUS;
> +	if (page_num > obj_pages)
> +		page_num = obj_pages;
> +
>  	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);

[Severity: High]
This is a pre-existing issue, but could this concurrent modification
corrupt page protection bits?

Modifying vma->vm_page_prot dynamically in the psb_fbdev_vm_fault handler
happens under a read lock, such as mmap_read_lock or the per-VMA read lock.

On 32-bit systems with PAE where pgprot_t is 64-bit, writing to it without
exclusive locks can cause torn writes. It also appears to race with other
concurrent faults or mprotect calls, potentially reverting security
boundaries or cache attributes since mm locking guidelines require holding
mmap_write_lock when modifying vma->vm_page_prot.

Should this assignment be moved to the mmap callback where mmap_write_lock
is held, rather than modifying it directly in the fault handler?

>  	for (i = 0; i < page_num; ++i) {
>  		err = vmf_insert_mixed(vma, address, pfn);
>  		if (unlikely(err & VM_FAULT_ERROR))

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806034343.43629-1-baul.lee@xbow.com?part=1