linux-kernel - Re: [RFC PATCH 10/10] vfio/type1: Register device notifier

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210225191714.GU4247@nvidia.com>
Date:   Thu, 25 Feb 2021 15:17:14 -0400
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Peter Xu <peterx@...hat.com>
CC:     Alex Williamson <alex.williamson@...hat.com>, <cohuck@...hat.com>,
        <kvm@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 10/10] vfio/type1: Register device notifier

On Thu, Feb 25, 2021 at 02:06:46PM -0500, Peter Xu wrote:

> Agreed.  I saw discussions around on redefining the vm_pgoff namespace, I can't
> say I followed that closely either, but yes it definitely makes sense to always
> use an unified namespace.  Maybe we should even comment it somewhere on how
> vm_pgoff is encoded?

Yes, it should be described, it is subtle
 
> > Correct. VFIO can map into the IOMMU PFNs it can get a reference
> > to. pin_user_pages() works for the majority, special VFIO VMAs cover
> > the rest, and everthing else must be blocked for security.
> 
> If we all agree that the current follow_pfn() should only apply to vfio
> internal vmas, 

I want to remvoe follow_pfn(). Internal VMAs can deduce the PFN from
the vm_pgoff, they don't need to do follow.

> then it seems we can drop it indeed, as long as the crash reported
> in 5cbf3264b would fail gracefully at e.g. VFIO_IOMMU_MAP_DMA rather
> than triggering a kernel warning somehow.

Yes, this will just fail the ioctl because pin_user_pages() failed and
the VMA was not VFIO.

> However I'm still confused on why it's more secure - the current process to do
> VFIO_IOMMU_MAP_DMA should at least has proper permission for everything to be
> setup, including the special vma, right?  Say, if the process can write to
> those memories, then shouldn't we also allow it to grant this write permission
> to other devices too?

It is a use-after-free. Once the PFN is programmed into the IOMMU it
becomes completely divorced from the VMA. Remember there is no
pin_user_page here, so the PFN has no reference count.

If the owner of the VMA decided to zap it or otherwise then the IOMMU
access keeps going - but now the owner thinks the PFN is free'd and
nobody is referencing it. Goes bad.

Jason