On Wed, 10 Apr 2024 17:55:25 +0200 David Hildenbrand <david@xxxxxxxxxx> wrote: > We currently miss to handle various cases, resulting in a dangerous > follow_pte() (previously follow_pfn()) usage. > > (1) We're not checking PTE write permissions. > > Maybe we should simply always require pte_write() like we do for > pin_user_pages_fast(FOLL_WRITE)? Hard to tell, so let's check for > ACRN_MEM_ACCESS_WRITE for now. > > (2) We're not rejecting refcounted pages. > > As we are not using MMU notifiers, messing with refcounted pages is > dangerous and can result in use-after-free. Let's make sure to reject them. > > (3) We are only looking at the first PTE of a bigger range. > > We only lookup a single PTE, but memmap->len may span a larger area. > Let's loop over all involved PTEs and make sure the PFN range is > actually contiguous. Reject everything else: it couldn't have worked > either way, and rather made use access PFNs we shouldn't be accessing. > This all sounds rather nasty and the maintainers of this driver may choose to turn your fixes into something suitable for current mainline and for -stable backporting. If they choose to do this then please just go ahead. Once such a change appear in linux-next the mm-unstable patch "virt: acrn: stop using follow_pfn" will start generating rejects, which will be easy enough to handle. Of they may choose to incorporate that change at the same time. Here it is: From: Christoph Hellwig <hch@xxxxxx> Subject: virt: acrn: stop using follow_pfn Date: Mon, 25 Mar 2024 07:45:40 +0800 Switch from follow_pfn to follow_pte so that we can get rid of follow_pfn. Note that this doesn't fix any of the pre-existing raciness and lack of permission checking in the code. Link: https://lkml.kernel.org/r/20240324234542.2038726-1-hch@xxxxxx Link: https://lkml.kernel.org/r/20240324234542.2038726-2-hch@xxxxxx Signed-off-by: Christoph Hellwig <hch@xxxxxx> Reviewed-by: David Hildenbrand <david@xxxxxxxxxx> Cc: Andy Lutomirski <luto@xxxxxxxxxx> Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx> Cc: Fei Li <fei1.li@xxxxxxxxx> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx> Cc: Ingo Molnar <mingo@xxxxxxxxxx> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> --- drivers/virt/acrn/mm.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) --- a/drivers/virt/acrn/mm.c~virt-acrn-stop-using-follow_pfn +++ a/drivers/virt/acrn/mm.c @@ -172,18 +172,24 @@ int acrn_vm_ram_map(struct acrn_vm *vm, mmap_read_lock(current->mm); vma = vma_lookup(current->mm, memmap->vma_base); if (vma && ((vma->vm_flags & VM_PFNMAP) != 0)) { + spinlock_t *ptl; + pte_t *ptep; + if ((memmap->vma_base + memmap->len) > vma->vm_end) { mmap_read_unlock(current->mm); return -EINVAL; } - ret = follow_pfn(vma, memmap->vma_base, &pfn); - mmap_read_unlock(current->mm); + ret = follow_pte(vma->vm_mm, memmap->vma_base, &ptep, &ptl); if (ret < 0) { + mmap_read_unlock(current->mm); dev_dbg(acrn_dev.this_device, "Failed to lookup PFN at VMA:%pK.\n", (void *)memmap->vma_base); return ret; } + pfn = pte_pfn(ptep_get(ptep)); + pte_unmap_unlock(ptep, ptl); + mmap_read_unlock(current->mm); return acrn_mm_region_add(vm, memmap->user_vm_pa, PFN_PHYS(pfn), memmap->len, _
