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;
Date:   Tue, 24 Nov 2020 10:07:51 -0700
From:   Alex Williamson <alex.williamson@redhat.com>
To:     Justin He <Justin.He@arm.com>
Cc:     Cornelia Huck <cohuck@redhat.com>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Peter Xu <peterx@redhat.com>
Subject: Re: [PATCH] vfio iommu type1: Bypass the vma permission check in
 vfio_pin_pages_remote()
Message-ID: <20201124100751.793c671f@w520.home>
In-Reply-To: <AM6PR08MB32248D873EDD8923675F2D3BF7FC0@AM6PR08MB3224.eurprd08.prod.outlook.com>
References: <20201119142737.17574-1-justin.he@arm.com>
        <20201119100508.483c6503@w520.home>
        <AM6PR08MB32248D873EDD8923675F2D3BF7FC0@AM6PR08MB3224.eurprd08.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 23 Nov 2020 02:37:32 +0000
Justin He <Justin.He@arm.com> wrote:

> Hi Alex, thanks for the comments.
> See mine below:
> 
> > -----Original Message-----
> > From: Alex Williamson <alex.williamson@redhat.com>
> > Sent: Friday, November 20, 2020 1:05 AM
> > To: Justin He <Justin.He@arm.com>
> > Cc: Cornelia Huck <cohuck@redhat.com>; kvm@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH] vfio iommu type1: Bypass the vma permission check in
> > vfio_pin_pages_remote()
> >
> > On Thu, 19 Nov 2020 22:27:37 +0800
> > Jia He <justin.he@arm.com> wrote:
> >  
> > > The permission of vfio iommu is different and incompatible with vma
> > > permission. If the iotlb->perm is IOMMU_NONE (e.g. qemu side), qemu will
> > > simply call unmap ioctl() instead of mapping. Hence vfio_dma_map() can't
> > > map a dma region with NONE permission.
> > >
> > > This corner case will be exposed in coming virtio_fs cache_size
> > > commit [1]
> > >  - mmap(NULL, size, PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> > >    memory_region_init_ram_ptr()
> > >  - re-mmap the above area with read/write authority.
> > >  - vfio_dma_map() will be invoked when vfio device is hotplug added.
> > >
> > > qemu:
> > > vfio_listener_region_add()
> > > vfio_dma_map(..., readonly=false)
> > > map.flags is set to VFIO_DMA_MAP_FLAG_READ|VFIO_..._WRITE
> > > ioctl(VFIO_IOMMU_MAP_DMA)
> > >
> > > kernel:
> > > vfio_dma_do_map()
> > > vfio_pin_map_dma()
> > > vfio_pin_pages_remote()
> > > vaddr_get_pfn()
> > > ...
> > > check_vma_flags() failed! because
> > > vm_flags hasn't VM_WRITE && gup_flags
> > > has FOLL_WRITE
> > >
> > > It will report error in qemu log when hotplug adding(vfio) a nvme disk
> > > to qemu guest on an Ampere EMAG server:
> > > "VFIO_MAP_DMA failed: Bad address"  
> >
> > I don't fully understand the argument here, I think this is suggesting
> > that because QEMU won't call VFIO_IOMMU_MAP_DMA on a region that has
> > NONE permission, the kernel can ignore read/write permission by using
> > FOLL_FORCE.  Not only is QEMU not the only userspace driver for vfio,
> > but regardless of that, we can't trust the behavior of any given
> > userspace driver.  Bypassing the permission check with FOLL_FORCE seems
> > like it's placing the trust in the user, which seems like a security
> > issue.  Thanks,  
> Yes, this might have side impact on security.
> But besides this simple fix(adding FOLL_FORCE), do you think it is a good
> idea that:
> Qemu provides a special vfio_dma_map_none_perm() to allow mapping a
> region with NONE permission?

If NONE permission implies that we use FOLL_FORCE as described here to
ignore the r+w permissions and trust that the user knows what they're
doing, that seems like a non-starter.  Ultimately I think what you're
describing is a scenario where our current permission check fails and
the solution is probably to extend the check to account for other ways
that a user may have access to a vma rather than bypass the check.
Thanks,

Alex