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

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 <[email protected]>

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/[email protected]?part=1