Re: [PATCH v2 2/6] drm/shmem_helper: use vmf_insert_pfn_mkwrite()

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

Pre-existing issues:
- [Critical] The `.huge_fault` handler `drm_gem_shmem_any_fault` does not verify if the PMD-sized huge page fits entirely within the bounds of the VMA, allowing a VMA boundary bypass.
- [High] Incorrect page offset calculation in DRM shmem fault handlers causes the wrong pages to be mapped.
--

--- Patch [2]: [PATCH v2 2/6] drm/shmem_helper: use vmf_insert_pfn_mkwrite() ---
commit 6ce5b52de7395aa4c0a50d399649686c52e613a9
Author: Paolo Bonzini <[email protected]>

drm/shmem_helper: use vmf_insert_pfn_mkwrite()

This ensures that KVM or VFIO correctly see a writable PTE when
they request one. Otherwise, a guest write to an unpopulated
PTE from a mapping backed by a DRM GEM BO triggers a VM exit
with EFAULT.

Link: https://lore.kernel.org/kvm/[email protected]/

> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index c989459eb2159..c81be3e973173 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -589,11 +589,25 @@ static void drm_gem_shmem_record_mkwrite(struct vm_fault *vmf)
>  	folio_mark_dirty(page_folio(shmem->pages[page_offset]));

[Severity: High]
This is a pre-existing issue, but does this page_offset calculation
correctly handle the requested offset within the GEM object?

When userspace mmaps a sub-range of a GEM object with a non-zero
offset, or unmaps a prefix of the VMA, the core memory management
sets vma->vm_pgoff to include the requested offset. Because
page_offset evaluates to vmf->pgoff - vma->vm_pgoff, it calculates
the offset relative to the VMA start, ignoring the requested offset
within the GEM object itself.

This can cause folio_mark_dirty() to mark the wrong folio dirty.
The same calculation is present in drm_gem_shmem_any_fault(),
which could lead to mapping the wrong physical pages.

>  }
>  
> +/*
> + * Because the vm_ops have a .pfn_mkwrite() callback, vma_set_page_prot()
> + * has cleared the write bit from vma->vm_page_prot.  vmf_insert_pfn()
> + * would install a read-only entry even for a write fault, relying on a
> + * second fault to reach .pfn_mkwrite() and upgrade it, but that second
> + * fault never happens for fixup_user_fault() callers that directly
> + * walk the page tables with follow_pfnmap_start().  To ensure that
> + * they don't see the read-only entry, pass FAULT_FLAG_WRITE info down
> + * to install a writable entry right away.  Because .pfn_mkwrite() is
> + * not invoked, record the write afterwards.
> + */
>  static vm_fault_t try_insert_pfn(struct vm_fault *vmf, unsigned int order,
>  				 unsigned long pfn)
>  {
> +	bool write = vmf->flags & FAULT_FLAG_WRITE;
> +	vm_fault_t ret = VM_FAULT_FALLBACK;
> +
>  	if (!order) {
> -		return vmf_insert_pfn(vmf->vma, vmf->address, pfn);
> +		ret = vmf_insert_pfn_mkwrite(vmf->vma, vmf->address, pfn, write);
>  #ifdef CONFIG_ARCH_SUPPORTS_PMD_PFNMAP
>  	} else if (order == PMD_ORDER) {
>  		unsigned long paddr = pfn << PAGE_SHIFT;
> @@ -601,27 +615,15 @@ static vm_fault_t try_insert_pfn(struct vm_fault *vmf, unsigned int order,
>  
>  		if (aligned &&
>  		    folio_test_pmd_mappable(page_folio(pfn_to_page(pfn)))) {
> -			vm_fault_t ret;
> -
>  			pfn &= PMD_MASK >> PAGE_SHIFT;
> -
> -			/* Unlike PTEs which are automatically upgraded to
> -			 * writeable entries, the PMD upgrades go through
> -			 * .huge_fault(). Make sure we pass the "write" info
> -			 * along in that case.
> -			 * This also means we have to record the write fault
> -			 * here, instead of in .pfn_mkwrite().
> -			 */
> -			ret = vmf_insert_pfn_pmd(vmf, pfn,
> -						 vmf->flags & FAULT_FLAG_WRITE);
> -			if (ret == VM_FAULT_NOPAGE && (vmf->flags & FAULT_FLAG_WRITE))
> -				drm_gem_shmem_record_mkwrite(vmf);
> -
> -			return ret;
> +			ret = vmf_insert_pfn_pmd(vmf, pfn, write);

[Severity: Critical]
This is a pre-existing issue, but does the fault handler verify that a
PMD-sized huge page fits entirely within the bounds of the VMA?

The core __thp_vma_allowable_orders() skips VMA boundary checks by
relying on the driver's .huge_fault handler to enforce them. However,
drm_gem_shmem_any_fault() does not appear to check if
vmf->address + PMD_SIZE <= vma->vm_end before delegating to
vmf_insert_pfn_pmd() via try_insert_pfn().

Could an unprivileged user create a 4KB VMA backed by a PMD-aligned GEM
object, and fault it to install a 2MB PMD that overwrites adjacent VMA
page tables?

>  		}
>  #endif
>  	}
> -	return VM_FAULT_FALLBACK;
> +
> +	if (ret == VM_FAULT_NOPAGE && write)
> +		drm_gem_shmem_record_mkwrite(vmf);
> +	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2