Re: [PATCH 5/7] PCI/TSM: Add Device Security (TVM Guest) operations support

[<dan.j.williams@intel.com>]

Aneesh Kumar K.V wrote:
> Dan Williams <dan.j.williams@intel.com> writes:
> 
> > PCIe Trusted Execution Environment Device Interface Security Protocol
> > (TDISP) has two distinct sets of operations. The first, currently enabled
> > in driver/pci/tsm.c, enables the VMM to authenticate the physical function
> > (PCIe Component Measurement and Authentication (CMA)), establish a secure
> > message passing session (DMTF SPDM), and establish physical link security
> > (PCIe Integrity and Data Encryption (IDE)). The second set lets the TVM
> > manage the security state of assigned devices (TEE Device Interfaces
> > (TDIs)). Enable the latter with three new 'struct pci_tsm_ops' operations:
> >
> >  - lock(): Transition the device to the TDISP state. In this mode
> >    the device is responsible for validating that it is in a secure
> >    configuration and will transition to the TDISP ERROR state if those
> >    settings are modified. Device Security Manager (DSM) and the TEE
> >    Security Manager (TSM) enforce that the device is not permitted to issue
> >    T=1 traffic in this mode.
> >
[..]
> > @@ -453,6 +477,265 @@ static ssize_t disconnect_store(struct device *dev,
> >  }
> >  static DEVICE_ATTR_WO(disconnect);
> >  
> > +static struct resource **alloc_encrypted_resources(struct pci_dev *pdev,
> > +						   struct resource **__res)
> > +{
> > +	int i;
> > +
> > +	memset(__res, 0, sizeof(struct resource *) * PCI_NUM_RESOURCES);
> > +
> > +	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> > +		unsigned long flags = pci_resource_flags(pdev, i);
> > +		resource_size_t len = pci_resource_len(pdev, i);
> > +
> > +		if (!len || !(flags & IORESOURCE_MEM))
> > +			continue;
> > +
> > +
> > +		__res[i] = kzalloc(sizeof(struct resource), GFP_KERNEL);
> > +		if (!__res[i])
> > +			break;
> > +
> > +		*__res[i] = DEFINE_RES_NAMED_DESC(pci_resource_start(pdev, i),
> > +						  len, "PCI MMIO Encrypted",
> > +						  flags, IORES_DESC_ENCRYPTED);
> > +
> >
> 
> Not all resources are secure/encrypted. For example, if secure
> interrupts are not supported, then the MSI-X table and PBA BARs remain
> shared resources between the guest and the hypervisor.

I failed to mention this in the changelog, but part of thinking here is
that the mixed security state of a device based on interface report I
was expecting to be follow-on work. I.e. that minimum viable base case
is all MMIO is private of a private PCI device.

However, we can still keep the PCI/TSM core implementation equally
simple by moving these iomem manipulation routines to library helpers
that a TSM driver can optionally call as part of ->accept().

> In ARM CCA, the interface report is used to determine the nature of each
> region. When ioremap() is called, it relies on the Realm IPA state of
> the guest physical address to decide whether the mapping should be
> treated as shared or private [1], [2].
> 
> With this change we report the msix table and pba range as encrypted in
> /proc/iomem
> 
>  50012000-50012fff : PCI MMIO Encrypted
>     50012000-50012fff : 0000:00:01.0
>       50012000-50012fff : ahci
>   50013000-50013fff : PCI MMIO Encrypted
>     50013000-50013fff : 0000:00:01.0
>       50013000-50013fff : ahci

Unless these interface reports from different archs are all coming back
in a common format that can be parsed I do think it is a good idea to
reflect private MMIO in /proc/iomem regardless of how the arch
communicates to ioremap() to apply the private mapping pgprot setting.

In the meantime I will move these to optional library calls.