Messages in this thread | | | Subject | Re: [PATCH RFC 6/9] s390/pci_mmio: fully validate the VMA before calling follow_pte() | From | David Hildenbrand <> | Date | Fri, 10 Sep 2021 11:23:48 +0200 |
| |
On 10.09.21 10:22, Niklas Schnelle wrote: > On Thu, 2021-09-09 at 16:59 +0200, David Hildenbrand wrote: >> We should not walk/touch page tables outside of VMA boundaries when >> holding only the mmap sem in read mode. Evil user space can modify the >> VMA layout just before this function runs and e.g., trigger races with >> page table removal code since commit dd2283f2605e ("mm: mmap: zap pages >> with read mmap_sem in munmap"). >> >> find_vma() does not check if the address is >= the VMA start address; >> use vma_lookup() instead. >> >> Fixes: dd2283f2605e ("mm: mmap: zap pages with read mmap_sem in munmap") >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> arch/s390/pci/pci_mmio.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/s390/pci/pci_mmio.c b/arch/s390/pci/pci_mmio.c >> index ae683aa623ac..c5b35ea129cf 100644 >> --- a/arch/s390/pci/pci_mmio.c >> +++ b/arch/s390/pci/pci_mmio.c >> @@ -159,7 +159,7 @@ SYSCALL_DEFINE3(s390_pci_mmio_write, unsigned long, mmio_addr, >> >> mmap_read_lock(current->mm); >> ret = -EINVAL; >> - vma = find_vma(current->mm, mmio_addr); >> + vma = vma_lookup(current->mm, mmio_addr); >> if (!vma) >> goto out_unlock_mmap; >> if (!(vma->vm_flags & (VM_IO | VM_PFNMAP))) >> @@ -298,7 +298,7 @@ SYSCALL_DEFINE3(s390_pci_mmio_read, unsigned long, mmio_addr, >> >> mmap_read_lock(current->mm); >> ret = -EINVAL; >> - vma = find_vma(current->mm, mmio_addr); >> + vma = vma_lookup(current->mm, mmio_addr); >> if (!vma) >> goto out_unlock_mmap; >> if (!(vma->vm_flags & (VM_IO | VM_PFNMAP))) > > Oh wow great find thanks! If I may say so these are not great function > names. Looking at the code vma_lookup() is inded find_vma() plus the > check that the looked up address is indeed inside the vma. >
IIRC, vma_lookup() was introduced fairly recently. Before that, this additional check was open coded (and still are in some instances). It's confusing, I agree.
> I think this is pretty independent of the rest of the patches, so do > you want me to apply this patch independently or do you want to wait > for the others?
Sure, please go ahead and apply independently. It'd be great if you could give it a quick sanity test, although I don't expect surprises -- unfortunately, the environment I have easily at hand is not very well suited (#cpu, #mem, #disk ...) for anything that exceeds basic compile tests (and even cross-compiling is significantly faster ...).
> > In any case: > > Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com> >
Thanks!
-- Thanks,
David / dhildenb
| |