All callers of follow_pfnmap_start() except s390_pci_mmio_write() are following it, if they are doing a write, with a check that args.writable is true; for s390_pci_mmio_write() that's a bug. Also, most of them return -EFAULT if it is not. Pull the check directly into follow_pfnmap_start() through another input parameter args.write_fault, to eliminate the need to do it in the caller.
This also fixes an issue where follow_pfnmap_start() would return 0 for a PFN that is mapped read-only, and the caller would not attempt to call fixup_user_fault() on it; this can happen with vm_ops that set .pfn_mkwrite(), for example. Instead, now the caller (for example hva_to_pfn_remapped()) sees an error, does attempt to fix it, and only returns -EFAULT if the fixup was fruitless. Reported-by: Sergio Lopez <[email protected]> Fixes: 28e3918179aa ("drm/gem-shmem: Track folio accessed/dirty status in mmap") Link: https://lore.kernel.org/kvm/CAAiTLFU1ALsDoJoKW3d9bUvv990AozAoX=behmfng54qyba...@mail.gmail.com/ Cc: [email protected] Signed-off-by: Paolo Bonzini <[email protected]> --- arch/s390/pci/pci_mmio.c | 2 ++ drivers/vfio/vfio_iommu_type1.c | 17 +++++---- drivers/virt/acrn/mm.c | 10 +----- include/linux/mm.h | 3 ++ mm/memory.c | 62 ++++++++++++++++++++------------- virt/kvm/kvm_main.c | 15 ++++---- 6 files changed, 58 insertions(+), 51 deletions(-) diff --git a/arch/s390/pci/pci_mmio.c b/arch/s390/pci/pci_mmio.c index 51e7a28af899..d9d5b3318cbc 100644 --- a/arch/s390/pci/pci_mmio.c +++ b/arch/s390/pci/pci_mmio.c @@ -180,6 +180,7 @@ SYSCALL_DEFINE3(s390_pci_mmio_write, unsigned long, mmio_addr, args.address = mmio_addr; args.vma = vma; + args.write = true; ret = follow_pfnmap_start(&args); if (ret) { fixup_user_fault(current->mm, mmio_addr, FAULT_FLAG_WRITE, NULL); @@ -332,6 +333,7 @@ SYSCALL_DEFINE3(s390_pci_mmio_read, unsigned long, mmio_addr, args.vma = vma; args.address = mmio_addr; + args.write = false; ret = follow_pfnmap_start(&args); if (ret) { fixup_user_fault(current->mm, mmio_addr, 0, NULL); diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index c8151ba54de3..e6d3a2311a99 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -541,7 +541,11 @@ static int follow_fault_pfn(struct vm_area_struct *vma, struct mm_struct *mm, unsigned long vaddr, unsigned long *pfn, unsigned long *addr_mask, bool write_fault) { - struct follow_pfnmap_args args = { .vma = vma, .address = vaddr }; + struct follow_pfnmap_args args = { + .vma = vma, + .address = vaddr, + .write = write_fault, + }; int ret; ret = follow_pfnmap_start(&args); @@ -563,15 +567,10 @@ static int follow_fault_pfn(struct vm_area_struct *vma, struct mm_struct *mm, return ret; } - if (write_fault && !args.writable) { - ret = -EFAULT; - } else { - *pfn = args.pfn; - *addr_mask = args.addr_mask; - } - + *pfn = args.pfn; + *addr_mask = args.addr_mask; follow_pfnmap_end(&args); - return ret; + return 0; } /* diff --git a/drivers/virt/acrn/mm.c b/drivers/virt/acrn/mm.c index 5bca500a83e0..2f9808399f19 100644 --- a/drivers/virt/acrn/mm.c +++ b/drivers/virt/acrn/mm.c @@ -177,7 +177,6 @@ int acrn_vm_ram_map(struct acrn_vm *vm, struct acrn_vm_memmap *memmap) vma = vma_lookup(current->mm, memmap->vma_base); if (vma && ((vma->vm_flags & VM_PFNMAP) != 0)) { unsigned long start_pfn, cur_pfn; - bool writable; if ((memmap->vma_base + memmap->len) > vma->vm_end) { mmap_read_unlock(current->mm); @@ -188,6 +187,7 @@ int acrn_vm_ram_map(struct acrn_vm *vm, struct acrn_vm_memmap *memmap) struct follow_pfnmap_args args = { .vma = vma, .address = memmap->vma_base + i * PAGE_SIZE, + .write = !!(memmap->attr & ACRN_MEM_ACCESS_WRITE), }; ret = follow_pfnmap_start(&args); @@ -197,16 +197,8 @@ int acrn_vm_ram_map(struct acrn_vm *vm, struct acrn_vm_memmap *memmap) cur_pfn = args.pfn; if (i == 0) start_pfn = cur_pfn; - writable = args.writable; follow_pfnmap_end(&args); - /* Disallow write access if the PTE is not writable. */ - if (!writable && - (memmap->attr & ACRN_MEM_ACCESS_WRITE)) { - ret = -EFAULT; - break; - } - /* Disallow refcounted pages. */ if (pfn_valid(cur_pfn) && !PageReserved(pfn_to_page(cur_pfn))) { diff --git a/include/linux/mm.h b/include/linux/mm.h index 01184a4bdd6f..1659cb8f42fd 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -3136,9 +3136,12 @@ struct follow_pfnmap_args { * Inputs: * @vma: Pointer to @vm_area_struct struct * @address: the virtual address to walk + * @write: if true, fail with -EFAULT unless the mapping is + * writable */ struct vm_area_struct *vma; unsigned long address; + bool write; /** * Internals: * diff --git a/mm/memory.c b/mm/memory.c index b5555217b121..27f5dcc319c8 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -6774,12 +6774,15 @@ int __pmd_alloc(struct mm_struct *mm, pud_t *pud, unsigned long address) } #endif /* __PAGETABLE_PMD_FOLDED */ -static inline void pfnmap_args_setup(struct follow_pfnmap_args *args, - spinlock_t *lock, pte_t *ptep, - pgprot_t pgprot, unsigned long pfn_base, - unsigned long addr_mask, bool writable, - bool special) +static inline int pfnmap_args_setup(struct follow_pfnmap_args *args, + spinlock_t *lock, pte_t *ptep, + pgprot_t pgprot, unsigned long pfn_base, + unsigned long addr_mask, bool writable, + bool special) { + if (!writable && args->write) + return -EFAULT; + args->lock = lock; args->ptep = ptep; args->pfn = pfn_base + ((args->address & ~addr_mask) >> PAGE_SHIFT); @@ -6787,6 +6790,7 @@ static inline void pfnmap_args_setup(struct follow_pfnmap_args *args, args->pgprot = pgprot; args->writable = writable; args->special = special; + return 0; } static inline void pfnmap_lockdep_assert(struct vm_area_struct *vma) @@ -6808,8 +6812,9 @@ static inline void pfnmap_lockdep_assert(struct vm_area_struct *vma) * @args: Pointer to struct @follow_pfnmap_args * * The caller needs to setup args->vma and args->address to point to the - * virtual address as the target of such lookup. On a successful return, - * the results will be put into other output fields. + * virtual address as the target of such lookup, and optionally set + * args->write to require a writable mapping. On a successful + * return, the results will be put into other output fields. * * After the caller finished using the fields, the caller must invoke * another follow_pfnmap_end() to proper releases the locks and resources @@ -6832,7 +6837,8 @@ static inline void pfnmap_lockdep_assert(struct vm_area_struct *vma) * * This function must not be used to modify PTE content. * - * Return: zero on success, negative otherwise. + * Return: zero on success, -EFAULT if @args->write was set but the + * mapping is not writable, -EINVAL if there is no mapping at all. */ int follow_pfnmap_start(struct follow_pfnmap_args *args) { @@ -6845,6 +6851,7 @@ int follow_pfnmap_start(struct follow_pfnmap_args *args) pud_t *pudp, pud; pmd_t *pmdp, pmd; pte_t *ptep, pte; + int r = -EINVAL; pfnmap_lockdep_assert(vma); @@ -6878,10 +6885,12 @@ int follow_pfnmap_start(struct follow_pfnmap_args *args) spin_unlock(lock); goto retry; } - pfnmap_args_setup(args, lock, NULL, pud_pgprot(pud), - pud_pfn(pud), PUD_MASK, pud_write(pud), - pud_special(pud)); - return 0; + r = pfnmap_args_setup(args, lock, NULL, pud_pgprot(pud), + pud_pfn(pud), PUD_MASK, pud_write(pud), + pud_special(pud)); + if (r) + spin_unlock(lock); + return r; } pmdp = pmd_offset(pudp, address); @@ -6899,10 +6908,12 @@ int follow_pfnmap_start(struct follow_pfnmap_args *args) spin_unlock(lock); goto retry; } - pfnmap_args_setup(args, lock, NULL, pmd_pgprot(pmd), - pmd_pfn(pmd), PMD_MASK, pmd_write(pmd), - pmd_special(pmd)); - return 0; + r = pfnmap_args_setup(args, lock, NULL, pmd_pgprot(pmd), + pmd_pfn(pmd), PMD_MASK, pmd_write(pmd), + pmd_special(pmd)); + if (r) + spin_unlock(lock); + return r; } ptep = pte_offset_map_lock(mm, pmdp, address, &lock); @@ -6911,14 +6922,16 @@ int follow_pfnmap_start(struct follow_pfnmap_args *args) pte = ptep_get(ptep); if (!pte_present(pte)) goto unlock; - pfnmap_args_setup(args, lock, ptep, pte_pgprot(pte), - pte_pfn(pte), PAGE_MASK, pte_write(pte), - pte_special(pte)); + r = pfnmap_args_setup(args, lock, ptep, pte_pgprot(pte), + pte_pfn(pte), PAGE_MASK, pte_write(pte), + pte_special(pte)); + if (r) + goto unlock; return 0; unlock: pte_unmap_unlock(ptep, lock); out: - return -EINVAL; + return r; } EXPORT_SYMBOL_GPL(follow_pfnmap_start); @@ -6960,7 +6973,11 @@ int generic_access_phys(struct vm_area_struct *vma, unsigned long addr, int offset = offset_in_page(addr); int ret = -EINVAL; bool writable; - struct follow_pfnmap_args args = { .vma = vma, .address = addr }; + struct follow_pfnmap_args args = { + .vma = vma, + .address = addr, + .write = !!(write & FOLL_WRITE) + }; retry: if (follow_pfnmap_start(&args)) @@ -6970,9 +6987,6 @@ int generic_access_phys(struct vm_area_struct *vma, unsigned long addr, writable = args.writable; follow_pfnmap_end(&args); - if ((write & FOLL_WRITE) && !writable) - return -EINVAL; - maddr = ioremap_prot(phys_addr, PAGE_ALIGN(len + offset), prot); if (!maddr) return -ENOMEM; diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 576bcb21be3a..b7c21a48a45c 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -2928,8 +2928,11 @@ static int hva_to_pfn_slow(struct kvm_follow_pfn *kfp, kvm_pfn_t *pfn) static int hva_to_pfn_remapped(struct vm_area_struct *vma, struct kvm_follow_pfn *kfp, kvm_pfn_t *p_pfn) { - struct follow_pfnmap_args args = { .vma = vma, .address = kfp->hva }; - bool write_fault = kfp->flags & FOLL_WRITE; + struct follow_pfnmap_args args = { + .vma = vma, + .address = kfp->hva, + .write = !!(kfp->flags & FOLL_WRITE), + }; int r; /* @@ -2948,7 +2951,7 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma, */ bool unlocked = false; r = fixup_user_fault(current->mm, kfp->hva, - (write_fault ? FAULT_FLAG_WRITE : 0), + (args.write ? FAULT_FLAG_WRITE : 0), &unlocked); if (unlocked) return -EAGAIN; @@ -2960,13 +2963,7 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma, return r; } - if (write_fault && !args.writable) { - *p_pfn = KVM_PFN_ERR_RO_FAULT; - goto out; - } - *p_pfn = kvm_resolve_pfn(kfp, NULL, &args, args.writable); -out: follow_pfnmap_end(&args); return r; } -- 2.55.0
