Virtualization - Aug 2021 - [PATCH v4 11/15] pci: Add pci_iomap_shared{,_range}

On Mon, Aug 23, 2021 at 07:14:18PM -0700, Andi Kleen
wrote:
> 
> On 8/23/2021 6:04 PM, Dan Williams wrote:
> > On Mon, Aug 23, 2021 at 5:31 PM Kuppuswamy, Sathyanarayanan
> > <sathyanarayanan.kuppuswamy at linux.intel.com> wrote:
> > > 
> > > 
> > > On 8/23/21 4:56 PM, Michael S. Tsirkin wrote:
> > > > > Add a new variant of pci_iomap for mapping all PCI
resources
> > > > > of a devices as shared memory with a hypervisor in a
confidential
> > > > > guest.
> > > > > 
> > > > > Signed-off-by: Andi Kleen<ak at linux.intel.com>
> > > > > Signed-off-by: Kuppuswamy
Sathyanarayanan<sathyanarayanan.kuppuswamy at linux.intel.com>
> > > > I'm a bit puzzled by this part. So why should the
guest*not*  map
> > > > pci memory as shared? And if the answer is never (as it
seems to be)
> > > > then why not just make regular pci_iomap DTRT?
> > > It is in the context of confidential guest (where VMM is
un-trusted). So
> > > we don't want to make all PCI resource as shared. It should
be allowed
> > > only for hardened drivers/devices.
> > That's confusing, isn't device authorization what keeps
unaudited
> > drivers from loading against untrusted devices? I'm feeling like
> > Michael that this should be a detail that drivers need not care about
> > explicitly, in which case it does not need to be exported because the
> > detail can be buried in lower levels.
> 
> We originally made it default (similar to AMD), but it during code audit we
> found a lot of drivers who do ioremap early outside the probe function.
> Since it would be difficult to change them all we made it opt-in, which
> ensures that only drivers that have been enabled can talk with the host at
> all and can't be attacked. That made the problem of hardening all these
> drivers a lot more practical.
> 
> Currently we only really need virtio and MSI-X shared, so for changing two
> places in the tree you avoid a lot of headache elsewhere.
> 
> Note there is still a command line option to override if you want to allow
> and load other drivers.
> 
> -Andi
I see. Hmm. It's a bit of a random thing to do it at the map time
though. E.g. DMA is all handled transparently behind the DMA API.
Hardening is much more than just replacing map with map_shared
and I suspect what you will end up with is basically
vendors replacing map with map shared to make things work
for their users and washing their hands.

I would say an explicit flag in the driver that says "hardened"
and refusing to init a non hardened one would be better.

-- 
MST

> I see. Hmm. It's a bit of a random thing to do it at the map time
> though. E.g. DMA is all handled transparently behind the DMA API.
> Hardening is much more than just replacing map with map_shared
> and I suspect what you will end up with is basically
> vendors replacing map with map shared to make things work
> for their users and washing their hands.
That concept exists too. There is a separate allow list for the drivers. 
So just adding shared to a driver is not enough, until it's also added 
to the allowlist

Users can of course chose to disable the allowlist, but they need to 
understand the security implications.

>
> I would say an explicit flag in the driver that says "hardened"
> and refusing to init a non hardened one would be better.

We have that too (that's the device filtering)

But the problem is that device filtering just stops the probe functions, 
not the initcalls, and lot of legacy drivers do MMIO interactions before 
going into probe. In some cases it's unavoidable because of the device 
doesn't have a separate enumeration mechanism it needs some kind of 
probing to even check for its existence And since we don't want to 
change all of them it's far safer to make the ioremap opt-in.


-Andi