linux-kernel - RE: [PATCH 4/5] vfio/type1: Flush CPU caches on DMA pages in non-coherent domains

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: <BN9PR11MB52769E209C5B978C7094A5C08CEB2@BN9PR11MB5276.namprd11.prod.outlook.com>
Date: Wed, 22 May 2024 06:24:14 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Jason Gunthorpe <jgg@...dia.com>, Alex Williamson
	<alex.williamson@...hat.com>
CC: "Vetter, Daniel" <daniel.vetter@...el.com>, "Zhao, Yan Y"
	<yan.y.zhao@...el.com>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"x86@...nel.org" <x86@...nel.org>, "iommu@...ts.linux.dev"
	<iommu@...ts.linux.dev>, "pbonzini@...hat.com" <pbonzini@...hat.com>,
	"seanjc@...gle.com" <seanjc@...gle.com>, "dave.hansen@...ux.intel.com"
	<dave.hansen@...ux.intel.com>, "luto@...nel.org" <luto@...nel.org>,
	"peterz@...radead.org" <peterz@...radead.org>, "tglx@...utronix.de"
	<tglx@...utronix.de>, "mingo@...hat.com" <mingo@...hat.com>, "bp@...en8.de"
	<bp@...en8.de>, "hpa@...or.com" <hpa@...or.com>, "corbet@....net"
	<corbet@....net>, "joro@...tes.org" <joro@...tes.org>, "will@...nel.org"
	<will@...nel.org>, "robin.murphy@....com" <robin.murphy@....com>,
	"baolu.lu@...ux.intel.com" <baolu.lu@...ux.intel.com>, "Liu, Yi L"
	<yi.l.liu@...el.com>
Subject: RE: [PATCH 4/5] vfio/type1: Flush CPU caches on DMA pages in
 non-coherent domains

> From: Jason Gunthorpe <jgg@...dia.com>
> Sent: Wednesday, May 22, 2024 2:38 AM
> 
> On Tue, May 21, 2024 at 12:19:45PM -0600, Alex Williamson wrote:
> > > I'm OK with this. If devices are insecure then they need quirks in
> > > vfio to disclose their problems, we shouldn't punish everyone who
> > > followed the spec because of some bad actors.
> > >
> > > But more broadly in a security engineered environment we can trust the
> > > no-snoop bit to work properly.
> >
> >  The spec has an interesting requirement on devices sending no-snoop
> >  transactions anyway (regarding PCI_EXP_DEVCTL_NOSNOOP_EN):
> >
> >  "Even when this bit is Set, a Function is only permitted to Set the No
> >   Snoop attribute on a transaction when it can guarantee that the
> >   address of the transaction is not stored in any cache in the system."
> >
> > I wouldn't think the function itself has such visibility and it would
> > leave the problem of reestablishing coherency to the driver, but am I
> > overlooking something that implicitly makes this safe?
> 
> I think it is just bad spec language! People are clearly using
> no-snoop on cachable memory today. The authors must have had some
> other usage in mind than what the industry actually did.

sure no-snoop can be used on cacheable memory but then the driver
needs to flush the cache before triggering the no-snoop DMA so it
still meets the spec "the address of the transaction is not stored
in any cache in the system".

but as Alex said the function itself has no such visibility so it's really
a guarantee made by the driver.

> > > That is where this series is, it assumes a no-snoop transaction took
> > > place even if that is impossible, because of config space, and then
> > > does pessimistic flushes.
> >
> > So are you proposing that we can trust devices to honor the
> > PCI_EXP_DEVCTL_NOSNOOP_EN bit and virtualize it to be hardwired to
> zero
> > on IOMMUs that do not enforce coherency as the entire solution?
> 
> Maybe not entire, but as an additional step to reduce the cost of
> this. ARM would like this for instance.

I searched PCI_EXP_DEVCTL_NOSNOOP_EN but surprisingly it's not
touched by i915 driver. sort of suggesting that Intel GPU doesn't follow
the spec to honor that bit...

> 
> > Or maybe we trap on setting the bit to make the flushing less
> > pessimistic?
> 
> Also a good idea. The VMM could then decide on policy.
> 

On Intel platform there is no pessimistic flush. Only Intel GPUs are
exempted from IOMMU force snoop (either being lacking of the
capability on the IOMMU dedicated to the GPU or having a special
flag bit < REQ_WO_PASID_PGSNP_NOTALLOWED> in the ACPI
structure for the IOMMU hosting many devices) to require the
additional flushes in this series.

We just need to avoid such flushes on other platforms e.g. ARM.

I'm fine to do a special check in the attach path to enable the flush
only for Intel GPU.

or alternatively could ARM SMMU driver implement
@enforce_cache_coherency by disabling PCI nosnoop cap when
the SMMU itself cannot force snoop? Then VFIO/IOMMUFD could
still check enforce_cache_coherency generally to apply the cache
flush trick... 😊