Re: [PATCH 13/13] vfio/type1: Mark follow_pfn as unsafe
On Wed, Oct 07, 2020 at 08:14:06PM +0200, Daniel Vetter wrote: > 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? Ah, the patches Alex was working on had the refcount I think, it does need co-ordination across multiple VFIO instances IIRC. At least a simple check would guarentee we only have exposed PCI BAR pages which is not as bad security wise as the other stuff. Jason ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 13/13] vfio/type1: Mark follow_pfn as unsafe
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
> Cc: Jason Gunthorpe
> Cc: Kees Cook
> Cc: Dan Williams
> Cc: Andrew Morton
> Cc: John Hubbard
> Cc: Jérôme Glisse
> Cc: Jan Kara
> Cc: Dan Williams
> Cc: linux...@kvack.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-samsung-...@vger.kernel.org
> Cc: linux-me...@vger.kernel.org
> Cc: Alex Williamson
> Cc: Cornelia Huck
> Cc: k...@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.
Jason
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 13/13] vfio/type1: Mark follow_pfn as unsafe
On Wed, Oct 7, 2020 at 7:39 PM Jason Gunthorpe 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
> > Cc: Jason Gunthorpe
> > Cc: Kees Cook
> > Cc: Dan Williams
> > Cc: Andrew Morton
> > Cc: John Hubbard
> > Cc: Jérôme Glisse
> > Cc: Jan Kara
> > Cc: Dan Williams
> > Cc: linux...@kvack.org
> > Cc: linux-arm-ker...@lists.infradead.org
> > Cc: linux-samsung-...@vger.kernel.org
> > Cc: linux-me...@vger.kernel.org
> > Cc: Alex Williamson
> > Cc: Cornelia Huck
> > Cc: k...@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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 13/13] vfio/type1: Mark follow_pfn as unsafe
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
Cc: Jason Gunthorpe
Cc: Kees Cook
Cc: Dan Williams
Cc: Andrew Morton
Cc: John Hubbard
Cc: Jérôme Glisse
Cc: Jan Kara
Cc: Dan Williams
Cc: linux...@kvack.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-samsung-...@vger.kernel.org
Cc: linux-me...@vger.kernel.org
Cc: Alex Williamson
Cc: Cornelia Huck
Cc: k...@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);
}
return ret;
--
2.28.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
