Re: [PATCH RFC 6/9] s390/pci_mmio: fully validate the VMA before calling follow_pte()

[David Hildenbrand]

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@xxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxx>

Thanks!

--
Thanks,

David / dhildenb