Re: [PATCH v4 11/15] pci: Add pci_iomap_shared{,_range}

On Tue, Aug 24, 2021 at 10:04:26AM -0700, Andi Kleen wrote:

On 8/24/2021 12:07 AM, Christoph Hellwig wrote:

quoted

On Mon, Aug 23, 2021 at 05:30:54PM -0700, Kuppuswamy, Sathyanarayanan wrote:

quoted

On 8/23/21 4:56 PM, Michael S. Tsirkin wrote:

quoted

quoted

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<redacted>
Signed-off-by: Kuppuswamy Sathyanarayanan<sathyanarayanan.kuppuswamy@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.

Well, assuming the host can do any damage when mapped shared that also
means not mapping it shared will completely break the drivers.

There are several cases:

- We have driver filtering active to protect you against attacks from the
host against unhardened drivers.

In this case the drivers not working is the intended behavior.

- There is an command allow list override for some new driver, but the
driver is hardened and shared

The other drivers will still not work, but that's also the intended behavior

- Driver filtering is disabled or the allow list override is used to enable
some non hardened/enabled driver

There is a command line option to override the ioremap sharing default, it
will allow all drivers to do ioremap. We would really prefer to make it more
finegrained, but it's not possible in this case. Other drivers are likely
attackable.

- Driver filtering is disabled (allowing attacks on the drivers) and the
command line option for forced sharing is set.

All drivers initialize and can talk to the host through MMIO. Lots of
unhardened drivers are likely attackable.

-Andi

All this makes sense but ioremap is such a random place to declare
driver has been audited, and it's baked into the binary with no way for
userspace to set policy.

Again all we will end up with is gradual replacement of all ioremap
calls with ioremap_shared as people discover a given driver does not
work in a VM. How are you going to know driver has actually been
audited? what the quality of the audit was? did the people doing the
auditing understand what they are auditing for?  No way, right?
So IMHO, let it be for now.

-- 
MST