linux-kernel - Re: [PATCH v1 1/8] iommu: Add iommu_group_replace

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]

Date:   Mon, 14 Feb 2022 13:25:52 -0400
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Robin Murphy <robin.murphy@....com>
Cc:     kvm@...r.kernel.org, rafael@...nel.org,
        David Airlie <airlied@...ux.ie>, linux-pci@...r.kernel.org,
        Thierry Reding <thierry.reding@...il.com>,
        Diana Craciun <diana.craciun@....nxp.com>,
        Dmitry Osipenko <digetx@...il.com>,
        Will Deacon <will@...nel.org>, Ashok Raj <ashok.raj@...el.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Christoph Hellwig <hch@...radead.org>,
        Stuart Yoder <stuyoder@...il.com>,
        Kevin Tian <kevin.tian@...el.com>,
        Chaitanya Kulkarni <kch@...dia.com>,
        Alex Williamson <alex.williamson@...hat.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Cornelia Huck <cohuck@...hat.com>,
        linux-kernel@...r.kernel.org, Li Yang <leoyang.li@....com>,
        iommu@...ts.linux-foundation.org,
        Jacob jun Pan <jacob.jun.pan@...el.com>,
        Daniel Vetter <daniel@...ll.ch>
Subject: Re: [PATCH v1 1/8] iommu: Add iommu_group_replace_domain()

On Mon, Feb 14, 2022 at 04:38:23PM +0000, Robin Murphy wrote:

> > This works better because the iommu code can hold the internal group
> > while it finds the bus/device and then invokes the driver op. We don't
> > have a lifetime problem anymore under that lock.
> 
> That's certainly one of the cleaner possibilities - per the theme of this
> thread I'm not hugely keen on proliferating special VFIO-specific
> versions

IMHO this is still a net better than VFIO open coding buggy versions
as it has done.

> of IOMMU APIs, but trying to take the dev->mutex might be a bit heavy-handed
> and risky,

The container->group lock is held during this code, and the
container->group_lock is taken during probe under the
dev_mutex. Acquiring the dev_mutex inside the group_lock should not be
done.

> and getting at the vfio_group->device_lock a bit fiddly, so if I
> can't come up with anything nicer or more general it might be a fair
> compromise.

Actually that doesn't look so bad. A 'vfio allocate domain from group'
function that used the above trick looks OK to me right now.

If we could move the iommu_capable() test to a domain that would make
this pretty nice - getting the bus safely is a bit more of a PITA -
I'm much less keen on holding the device_lock for the whole function.

> > The remaining VFIO use of bus for iommu_capable() is better done
> > against the domain or the group object, as appropriate.
> 
> Indeed, although half the implementations of .capable are nonsense already,
> so I'm treating that one as a secondary priority for the moment (with an aim
> to come back afterwards and just try to kill it off as far as possible).
> RDMA and VFIO shouldn't be a serious concern for the kind of systems with
> heterogeneous IOMMUs at this point.

Well, lets see:

drivers/infiniband/hw/usnic/usnic_uiom.c:       if (!iommu_capable(dev->bus, IOMMU_CAP_CACHE_COHERENCY)) {
drivers/vhost/vdpa.c:   if (!iommu_capable(bus, IOMMU_CAP_CACHE_COHERENCY))

These are kind of hacky ways to say "userspace can actually do DMA
because we don't need privileged cache flush instructions on this
platform". I would love it if these could be moved to some struct
device API - I've aruged with Christoph a couple of times we need
something like that..

drivers/vfio/vfio_iommu_type1.c:        if (iommu_capable(bus, IOMMU_CAP_CACHE_COHERENCY))

This is doing the above, and also the no-snoop mess that Intel has
mixed in. How to exactly properly expose their special no-snoop
behavior is definitely something that should be on the domain.

drivers/pci/controller/vmd.c:   if (iommu_capable(vmd->dev->dev.bus, IOMMU_CAP_INTR_REMAP) ||
drivers/vfio/vfio_iommu_type1.c:                    iommu_capable(bus, IOMMU_CAP_INTR_REMAP);

Not sure about VMD, but the VFIO one is a security statement. It could
be quite happy as a domain query, or a flag 'require secure MSI
interrupts' as input to attach_domain.

> > > solving it on my own and end up deleting
> > > iommu_group_replace_domain() in about 6 months' time anyway.
> > 
> > I expect this API to remain until we figure out a solution to the PPC
> > problem, and come up with an alternative way to change the attached
> > domain on the fly.
> 
> I though PPC wasn't using the IOMMU API at all... or is that the problem?

It needs it both ways, a way to get all the DMA security properties
from Lu's series without currently using an iommu_domain to get
them. So the design is to attach a NULL domain for PPC and leave it
like that.

It is surely eventually fixable to introduce a domain to PPC, I would
just prefer we not make anything contingent on actually doing that. :\

Jason