Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <209614b6553374cef5fd306d4235a98472fc5e4d.camel@linux.ibm.com>
Subject: Re: [PATCH RFC 6/9] s390/pci_mmio: fully validate the VMA before
 calling follow_pte()
From:   Niklas Schnelle <schnelle@linux.ibm.com>
To:     Liam Howlett <liam.howlett@oracle.com>,
        David Hildenbrand <david@redhat.com>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-s390@vger.kernel.org" <linux-s390@vger.kernel.org>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>
Date:   Fri, 10 Sep 2021 16:31:22 +0200
In-Reply-To: <20210910141221.fuimjijydw57vxjz@revolver>
References: <20210909145945.12192-1-david@redhat.com>
         <20210909145945.12192-7-david@redhat.com>
         <82d683ec361245e1879b3f14492cdd5c41957e52.camel@linux.ibm.com>
         <d9ec2387-2645-796e-af47-26f22516f7fa@redhat.com>
         <20210910141221.fuimjijydw57vxjz@revolver>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Fri, 2021-09-10 at 14:12 +0000, Liam Howlett wrote:
> * David Hildenbrand <david@redhat.com> [210910 05:23]:
> > 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.
> 
> This confusion is why I introduced vma_lookup().  My hope is to reduce
> the users of find_vma() to only those that actually need the added
> functionality, which are mostly in the mm code.

Ah I see, soo the confusingly similar names are in hope of one day
making find_vma() only visible or at least used in the mm code. That
does make more sense then. Thanks for the explanation! Maybe this would
be a good candidate for a treewide change/coccinelle script? Then again
I guess sometimes one really wants find_vma() and it's hard to tell
apart.

> 

..snip..