On Wed, Oct 7, 2020 at 7:39 PM Jason Gunthorpe jgg@ziepe.ca wrote:
On Wed, Oct 07, 2020 at 06:44:26PM +0200, Daniel Vetter wrote:
The code seems to stuff these pfns into iommu pts (or something like that, I didn't follow), but there's no mmu_notifier to ensure that access is synchronized with pte updates.
Hence mark these as unsafe. This means that with CONFIG_STRICT_FOLLOW_PFN, these will be rejected.
Real fix is to wire up an mmu_notifier ... somehow. Probably means any invalidate is a fatal fault for this vfio device, but then this shouldn't ever happen if userspace is reasonable.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Jason Gunthorpe jgg@ziepe.ca Cc: Kees Cook keescook@chromium.org Cc: Dan Williams dan.j.williams@intel.com Cc: Andrew Morton akpm@linux-foundation.org Cc: John Hubbard jhubbard@nvidia.com Cc: Jérôme Glisse jglisse@redhat.com Cc: Jan Kara jack@suse.cz Cc: Dan Williams dan.j.williams@intel.com Cc: linux-mm@kvack.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-samsung-soc@vger.kernel.org Cc: linux-media@vger.kernel.org Cc: Alex Williamson alex.williamson@redhat.com Cc: Cornelia Huck cohuck@redhat.com Cc: kvm@vger.kernel.org
drivers/vfio/vfio_iommu_type1.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index 5fbf0c1f7433..a4d53f3d0a35 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -421,7 +421,7 @@ static int follow_fault_pfn(struct vm_area_struct *vma, struct mm_struct *mm, { int ret;
ret = follow_pfn(vma, vaddr, pfn);
ret = unsafe_follow_pfn(vma, vaddr, pfn); if (ret) { bool unlocked = false;@@ -435,7 +435,7 @@ static int follow_fault_pfn(struct vm_area_struct *vma, struct mm_struct *mm, if (ret) return ret;
ret = follow_pfn(vma, vaddr, pfn);
ret = unsafe_follow_pfn(vma, vaddr, pfn); }This is actually being commonly used, so it needs fixing.
When I talked to Alex about this last we had worked out a patch series that adds a test on vm_ops that the vma came from vfio in the first place. The VMA's created by VFIO are 'safe' as the PTEs are never changed.
Hm, but wouldn't need that the semi-nasty vma_open trick to make sure that vma doesn't untimely disappear? Or is the idea to look up the underlying vfio object, and refcount that directly? -Daniel