Re: [RFC PATCH 07/21] pci/tdisp: Introduce tsm module

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Fri, 23 Aug 2024 23:21:21 +1000
Alexey Kardashevskiy <aik@amd.com> wrote:

> The module responsibilities are:
> 1. detect TEE support in a device and create nodes in the device's sysfs
> entry;
> 2. allow binding a PCI device to a VM for passing it through in a trusted
> manner;
> 3. store measurements/certificates/reports and provide access to those for
> the userspace via sysfs.
> 
> This relies on the platform to register a set of callbacks,
> for both host and guest.
> 
> And tdi_enabled in the device struct.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>



Main thing missing here is a sysfs ABI doc.

Otherwise, some random comments as I get my head around it.

Jonathan


> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> new file mode 100644
> index 000000000000..d48eceaf5bc0
> --- /dev/null
> +++ b/include/linux/tsm.h
> @@ -0,0 +1,263 @@
...


> +/* Physical device descriptor responsible for IDE/TDISP setup */
> +struct tsm_dev {
> +	struct kref kref;
> +	const struct attribute_group *ag;
> +	struct pci_dev *pdev; /* Physical PCI function #0 */
> +	struct tsm_spdm spdm;
> +	struct mutex spdm_mutex;

Kind of obvious, but still good to document what data this is protecting.

> +
> +	u8 tc_mask;
> +	u8 cert_slot;
> +	u8 connected;
> +	struct {
> +		u8 enabled:1;
> +		u8 enable:1;
> +		u8 def:1;
> +		u8 dev_ide_cfg:1;
> +		u8 dev_tee_limited:1;
> +		u8 rootport_ide_cfg:1;
> +		u8 rootport_tee_limited:1;
> +		u8 id;
> +	} selective_ide[256];
> +	bool ide_pre;
> +
> +	struct tsm_blob *meas;
> +	struct tsm_blob *certs;
> +
> +	void *data; /* Platform specific data */
> +};
> +

> diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
> new file mode 100644
> index 000000000000..e90455a0267f
> --- /dev/null
> +++ b/drivers/virt/coco/tsm.c
...

>
> +struct tsm_tdi *tsm_tdi_get(struct device *dev)
> +{
> +	struct tsm_tdi *tdi = dev->tdi;
> +
> +	return tdi;
	return dev->tdi;
seems fine to me.

> +}
> +EXPORT_SYMBOL_GPL(tsm_tdi_get);
> +
> +static int spdm_forward(struct tsm_spdm *spdm, u8 type)
> +{
> +	struct pci_doe_mb *doe_mb;
> +	int rc;
> +
> +	if (type == PCI_DOE_PROTOCOL_SECURED_CMA_SPDM)
> +		doe_mb = spdm->doe_mb_secured;
> +	else if (type == PCI_DOE_PROTOCOL_CMA_SPDM)
> +		doe_mb = spdm->doe_mb;
> +	else
> +		return -EINVAL;
> +
> +	if (!doe_mb)
> +		return -EFAULT;
> +
> +	rc = pci_doe(doe_mb, PCI_VENDOR_ID_PCI_SIG, type,
> +		     spdm->req, spdm->req_len, spdm->rsp, spdm->rsp_len);
> +	if (rc >= 0)
	if (rc < 0)
		return rc;

	spdm->rsp_len = rc;
	
	return 0;

> +		spdm->rsp_len = rc;
> +
> +	return rc;
> +}
> +

> +
> +static int tsm_ide_refresh(struct tsm_dev *tdev, void *private_data)
> +{
> +	int ret;
> +
> +	if (!tsm.ops->ide_refresh)
> +		return -EPERM;
> +
> +	mutex_lock(&tdev->spdm_mutex);
guard() and early returns.

> +	while (1) {
> +		ret = tsm.ops->ide_refresh(tdev, private_data);
> +		if (ret <= 0)
> +			break;
> +
> +		ret = spdm_forward(&tdev->spdm, ret);
> +		if (ret < 0)
> +			break;
> +	}
> +	mutex_unlock(&tdev->spdm_mutex);
> +
> +	return ret;
> +}
> +
> +static void tsm_tdi_reclaim(struct tsm_tdi *tdi, void *private_data)
> +{
> +	int ret;
> +
> +	if (WARN_ON(!tsm.ops->tdi_reclaim))
> +		return;
> +
> +	mutex_lock(&tdi->tdev->spdm_mutex);
guard() and early returns.

> +	while (1) {
> +		ret = tsm.ops->tdi_reclaim(tdi, private_data);
> +		if (ret <= 0)
> +			break;
> +
> +		ret = spdm_forward(&tdi->tdev->spdm, ret);
> +		if (ret < 0)
> +			break;
> +	}
> +	mutex_unlock(&tdi->tdev->spdm_mutex);
> +}


> +static int tsm_tdi_status(struct tsm_tdi *tdi, void *private_data, struct tsm_tdi_status *ts)
> +{
> +	struct tsm_tdi_status tstmp = { 0 };
> +	int ret;
> +
> +	if (WARN_ON(!tsm.ops->tdi_status))
> +		return -EPERM;
> +
> +	mutex_lock(&tdi->tdev->spdm_mutex);
Perhaps scoped_guard() if it doesn't make sense to set *ts on error
(see below).
> +	while (1) {
> +		ret = tsm.ops->tdi_status(tdi, private_data, &tstmp);
> +		if (ret <= 0)
> +			break;
> +
> +		ret = spdm_forward(&tdi->tdev->spdm, ret);
> +		if (ret < 0)
> +			break;
> +	}
> +	mutex_unlock(&tdi->tdev->spdm_mutex);
> +
> +	*ts = tstmp;
Want to set this even on error?
> +
> +	return ret;
> +}


> +static ssize_t tsm_meas_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct tsm_dev *tdev = tsm_dev_get(dev);
> +	ssize_t n = 0;
> +
> +	if (!tdev->meas) {
> +		n = sysfs_emit(buf, "none\n");
> +	} else {
> +		if (!n)
Always true.

> +			n = tsm_meas_gen(tdev->meas, buf, PAGE_SIZE);
> +		if (!n)
> +			n = blob_show(tdev->meas, buf);
> +	}
> +
> +	tsm_dev_put(tdev);
> +	return n;
> +}
> +
> +static DEVICE_ATTR_RO(tsm_meas);

> +
> +static struct attribute *host_dev_attrs[] = {
> +	&dev_attr_tsm_cert_slot.attr,
> +	&dev_attr_tsm_tc_mask.attr,
> +	&dev_attr_tsm_dev_connect.attr,
> +	&dev_attr_tsm_sel_stream.attr,
> +	&dev_attr_tsm_ide_refresh.attr,
> +	&dev_attr_tsm_certs.attr,
> +	&dev_attr_tsm_meas.attr,
> +	&dev_attr_tsm_dev_status.attr,
> +	NULL,
That final comma not needed as we'll never add anything after
this.

> +};
> +static const struct attribute_group host_dev_group = {
> +	.attrs = host_dev_attrs,
> +};


> +
> +static ssize_t tsm_report_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct tsm_tdi *tdi = tsm_tdi_get(dev);
> +	ssize_t n = 0;
> +
> +	if (!tdi->report) {
> +		n = sysfs_emit(buf, "none\n");
> +	} else {
> +		if (!n)

Always true.

> +			n = tsm_report_gen(tdi->report, buf, PAGE_SIZE);
> +		if (!n)
> +			n = blob_show(tdi->report, buf);
> +	}
> +
> +	return n;

in all cases this is only set once and if set nothing else
happens. Just return directly at those.

> +}

> +static char *spdm_algos_to_str(u64 algos, char *buf, size_t len)
> +{
> +	size_t n = 0;
> +
> +	buf[0] = 0;
> +#define __ALGO(x) do {								\
> +		if ((n < len) && (algos & (1ULL << (TSM_TDI_SPDM_ALGOS_##x))))	\
> +			n += snprintf(buf + n, len - n, #x" ");			\
> +	} while (0)

Odd wrapping. I'd push the do { onto the next line and it will all look more normal.

> +
> +	__ALGO(DHE_SECP256R1);
> +	__ALGO(DHE_SECP384R1);
> +	__ALGO(AEAD_AES_128_GCM);
> +	__ALGO(AEAD_AES_256_GCM);
> +	__ALGO(ASYM_TPM_ALG_RSASSA_3072);
> +	__ALGO(ASYM_TPM_ALG_ECDSA_ECC_NIST_P256);
> +	__ALGO(ASYM_TPM_ALG_ECDSA_ECC_NIST_P384);
> +	__ALGO(HASH_TPM_ALG_SHA_256);
> +	__ALGO(HASH_TPM_ALG_SHA_384);
> +	__ALGO(KEY_SCHED_SPDM_KEY_SCHEDULE);
> +#undef __ALGO
> +	return buf;
> +}

> +
> +static ssize_t tsm_tdi_status_show(struct device *dev, struct device_attribute *attr, char *buf)
wrap
> +{

> +
> +static int tsm_tdi_init(struct tsm_dev *tdev, struct pci_dev *pdev)
> +{
> +	struct tsm_tdi *tdi;
> +	int ret = 0;
set in all paths that use it.

> +
> +	dev_info(&pdev->dev, "Initializing tdi\n");
> +	if (!tdev)
> +		return -ENODEV;
Is this defense needed?  Seems overkill given we just
got the tdev in all paths that lead here.

> +
> +	tdi = kzalloc(sizeof(*tdi), GFP_KERNEL);
> +	if (!tdi)
> +		return -ENOMEM;
> +
> +	/* tsm_dev_get() requires pdev->dev.tdi which is set later */
> +	if (!kref_get_unless_zero(&tdev->kref)) {
> +		ret = -EPERM;
> +		goto free_exit;
> +	}
> +
> +	if (tsm.ops->dev_connect)
> +		tdi->ag = &host_tdi_group;
> +	else
> +		tdi->ag = &guest_tdi_group;
> +
> +	ret = sysfs_create_link(&pdev->dev.kobj, &tdev->pdev->dev.kobj, "tsm_dev");
> +	if (ret)
> +		goto free_exit;
> +
> +	ret = device_add_group(&pdev->dev, tdi->ag);
> +	if (ret)
> +		goto sysfs_unlink_exit;
> +
> +	tdi->tdev = tdev;
> +	tdi->pdev = pci_dev_get(pdev);
> +
> +	pdev->dev.tdi_enabled = !pdev->is_physfn || tsm.physfn;
> +	pdev->dev.tdi = tdi;

As below, __free() + pointer steal will be neater here.

> +	pci_info(pdev, "TDI enabled=%d\n", pdev->dev.tdi_enabled);
> +
> +	return 0;
> +
> +sysfs_unlink_exit:
> +	sysfs_remove_link(&pdev->dev.kobj, "tsm_dev");
> +free_exit:
> +	kfree(tdi);
> +
> +	return ret;
> +}

> +static int tsm_dev_init(struct pci_dev *pdev, struct tsm_dev **ptdev)
> +{
> +	struct tsm_dev *tdev;
> +	int ret = 0;
> +
> +	dev_info(&pdev->dev, "Initializing tdev\n");

dev_dbg() for non RFC versions.

> +	tdev = kzalloc(sizeof(*tdev), GFP_KERNEL);
> +	if (!tdev)
> +		return -ENOMEM;
> +
> +	kref_init(&tdev->kref);
> +	tdev->tc_mask = tsm.tc_mask;
> +	tdev->cert_slot = tsm.cert_slot;
> +	tdev->pdev = pci_dev_get(pdev);
> +	mutex_init(&tdev->spdm_mutex);
> +
> +	if (tsm.ops->dev_connect)
> +		tdev->ag = &host_dev_group;
> +	else
> +		tdev->ag = &guest_dev_group;
> +
> +	ret = device_add_group(&pdev->dev, tdev->ag);
> +	if (ret)
> +		goto free_exit;
> +
> +	if (tsm.ops->dev_connect) {
> +		ret = -EPERM;
> +		tdev->pdev = pci_dev_get(pdev);
> +		tdev->spdm.doe_mb = pci_find_doe_mailbox(tdev->pdev,
> +							 PCI_VENDOR_ID_PCI_SIG,
> +							 PCI_DOE_PROTOCOL_CMA_SPDM);
> +		if (!tdev->spdm.doe_mb)
> +			goto pci_dev_put_exit;
Group not released
> +
> +		tdev->spdm.doe_mb_secured = pci_find_doe_mailbox(tdev->pdev,
> +								 PCI_VENDOR_ID_PCI_SIG,
> +								 PCI_DOE_PROTOCOL_SECURED_CMA_SPDM);
Long lines.  I'd wrap after =

> +		if (!tdev->spdm.doe_mb_secured)
> +			goto pci_dev_put_exit;
nor here.

> +	}
> +
> +	*ptdev = tdev;

Could use __free() magic for tdev and steal the ptr here.
Maybe not worth the effort though given you need the error block anyway.


> +	return 0;
> +
> +pci_dev_put_exit:
> +	pci_dev_put(pdev);
> +free_exit:
> +	kfree(tdev);
> +
> +	return ret;
> +}
> +
> +static void tsm_dev_free(struct kref *kref)
> +{
> +	struct tsm_dev *tdev = container_of(kref, struct tsm_dev, kref);
> +
> +	device_remove_group(&tdev->pdev->dev, tdev->ag);
> +
> +	if (tdev->connected)
> +		tsm_dev_reclaim(tdev, tsm.private_data);
> +
> +	dev_info(&tdev->pdev->dev, "Freeing TDEV\n");

dev_dbg() eventually but fine for RFC.

> +	pci_dev_put(tdev->pdev);
> +	kfree(tdev);
> +}
> +
> +static int tsm_alloc_device(struct pci_dev *pdev)
> +{
> +	int ret = 0;
> +
> +	/* It is guest VM == TVM */
> +	if (!tsm.ops->dev_connect) {
> +		if (pdev->devcap & PCI_EXP_DEVCAP_TEE_IO) {
> +			struct tsm_dev *tdev = NULL;
> +
> +			ret = tsm_dev_init(pdev, &tdev);
> +			if (ret)
> +				return ret;
> +
> +			ret = tsm_tdi_init(tdev, pdev);
> +			tsm_dev_put(tdev);
> +			return ret;
> +		}
> +		return 0;
> +	}
> +
> +	if (pdev->is_physfn && (PCI_FUNC(pdev->devfn) == 0) &&
> +	    (pdev->devcap & PCI_EXP_DEVCAP_TEE_IO)) {
> +		struct tsm_dev *tdev = NULL;
> +
Trivial but... One blank line 
> +
> +		ret = tsm_dev_init(pdev, &tdev);
> +		if (ret)
> +			return ret;
> +
> +		ret = tsm_tdi_init(tdev, pdev);
> +		tsm_dev_put(tdev);
> +		return ret;
> +	}
> +
> +	if (pdev->is_virtfn) {
> +		struct pci_dev *pf0 = pci_get_slot(pdev->physfn->bus,
> +						   pdev->physfn->devfn & ~7);
> +
> +		if (pf0 && (pf0->devcap & PCI_EXP_DEVCAP_TEE_IO)) {
> +			struct tsm_dev *tdev = tsm_dev_get(&pf0->dev);
> +
> +			ret = tsm_tdi_init(tdev, pdev);
> +			tsm_dev_put(tdev);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}

> +
> +static struct notifier_block tsm_pci_bus_nb = {
> +	.notifier_call = tsm_pci_bus_notifier,
> +};
> +
> +static int __init tsm_init(void)
> +{
> +	int ret = 0;
> +
> +	pr_info(DRIVER_DESC " version: " DRIVER_VERSION "\n");
> +	return ret;
> +}
> +
> +static void __exit tsm_cleanup(void)
> +{
> +}

These aren't needed. If it's a library module it will have
nothing to do on init / exit which is fine.
And we don't care about versions! If we do then the discoverability
of features etc is totally broken.


> +
> +void tsm_set_ops(struct tsm_ops *ops, void *private_data)
> +{
> +	struct pci_dev *pdev = NULL;
> +	int ret;
> +
> +	if (!tsm.ops && ops) {
> +		tsm.ops = ops;
> +		tsm.private_data = private_data;
> +
> +		for_each_pci_dev(pdev) {
> +			ret = tsm_alloc_device(pdev);
> +			if (ret)
> +				break;
> +		}
> +		bus_register_notifier(&pci_bus_type, &tsm_pci_bus_nb);
> +	} else {
> +		bus_unregister_notifier(&pci_bus_type, &tsm_pci_bus_nb);
> +		for_each_pci_dev(pdev)
> +			tsm_dev_freeice(&pdev->dev);
> +		tsm.ops = ops;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(tsm_set_ops);
> +
> +int tsm_tdi_bind(struct tsm_tdi *tdi, u32 guest_rid, u64 vmid, u32 asid)
> +{
> +	int ret;
> +
> +	if (WARN_ON(!tsm.ops->tdi_bind))
> +		return -EPERM;
> +
> +	tdi->guest_rid = guest_rid;
> +	tdi->vmid = vmid;
> +	tdi->asid = asid;
> +
> +	mutex_lock(&tdi->tdev->spdm_mutex);

scoped_guard() may help here and in the good path at least
allow a direct return.

> +	while (1) {
> +		ret = tsm.ops->tdi_bind(tdi, guest_rid, vmid, asid, tsm.private_data);
> +		if (ret < 0)
> +			break;
> +
> +		if (!ret)
> +			break;
> +
> +		ret = spdm_forward(&tdi->tdev->spdm, ret);
> +		if (ret < 0)
> +			break;
> +	}
> +	mutex_unlock(&tdi->tdev->spdm_mutex);
> +
> +	if (ret) {

I'd have separate err_unlock label and error handling path.
This pattern is somewhat harder to read.

> +		tsm_tdi_unbind(tdi);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(tsm_tdi_bind);
> +
> +void tsm_tdi_unbind(struct tsm_tdi *tdi)
> +{
> +	tsm_tdi_reclaim(tdi, tsm.private_data);
> +	tdi->vmid = 0;
> +	tdi->asid = 0;
> +	tdi->guest_rid = 0;
> +}
> +EXPORT_SYMBOL_GPL(tsm_tdi_unbind);
> +
> +int tsm_guest_request(struct tsm_tdi *tdi, enum tsm_tdisp_state *state, void *req_data)
> +{
> +	int ret;
> +
> +	if (!tsm.ops->guest_request)
> +		return -EPERM;
> +
> +	mutex_lock(&tdi->tdev->spdm_mutex);
guard(mutex)(&tdi->tdev->spmd_mutex);

Then you can do returns on error or finish instead of breaking out
just to unlock.



> +	while (1) {
> +		ret = tsm.ops->guest_request(tdi, tdi->guest_rid, tdi->vmid, req_data,
> +					     state, tsm.private_data);
> +		if (ret <= 0)
> +			break;
> +
> +		ret = spdm_forward(&tdi->tdev->spdm, ret);
> +		if (ret < 0)
> +			break;
> +	}
> +	mutex_unlock(&tdi->tdev->spdm_mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(tsm_guest_request);

> +
> +module_init(tsm_init);
Put these next to the relevant functions - or put the functions next to these.
> +module_exit(tsm_cleanup);
> +
> +MODULE_VERSION(DRIVER_VERSION);
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> diff --git a/Documentation/virt/coco/tsm.rst b/Documentation/virt/coco/tsm.rst
> new file mode 100644
> index 000000000000..3be6e8491e42
> --- /dev/null
> +++ b/Documentation/virt/coco/tsm.rst

I'd break this out as a separate patch - mostly to shout "DOCS here - read them"
as otherwise they end up at the end of a long email no one scrolls through.

> @@ -0,0 +1,62 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +What it is
> +==========
> +
> +This is for PCI passthrough in confidential computing (CoCo: SEV-SNP, TDX, CoVE).
> +Currently passing through PCI devices to a CoCo VM uses SWIOTLB to pre-shared
> +memory buffers.
> +
> +PCIe IDE (Integrity and Data Encryption) and TDISP (TEE Device Interface Security
> +Protocol) are protocols to enable encryption over PCIe link and DMA to encrypted
> +memory. This doc is focused to DMAing to encrypted VM, the encrypted host memory is
> +out of scope.
> +
> +
> +Protocols
> +=========
> +
> +PCIe r6 DOE is a mailbox protocol to read/write object from/to device.
> +Objects are of plain SPDM or secure SPDM type. SPDM is responsible for authenticating
> +devices, creating a secure link between a device and TSM.
> +IDE_KM manages PCIe link encryption keys, it works on top of secure SPDM.
> +TDISP manages a passed through PCI function state, also works on top on secure SPDM.
> +Additionally, PCIe defines IDE capability which provides the host OS a way
> +to enable streams on the PCIe link.
> +
> +
> +TSM module
> +==========
> +
> +This is common place to trigger device authentication and keys management.
> +It exposes certificates/measurenets/reports/status via sysfs and provides control

measurements 

> +over the link (limited though by the TSM capabilities).
> +A platform is expected to register a specific set of hooks. The same module works
> +in host and guest OS, the set of requires platform hooks is quite different.
> +
> +
> +Flow
> +====
> +
> +At the boot time the tsm.ko scans the PCI bus to find and setup TDISP-cabable
> +devices; it also listens to hotplug events. If setup was successful, tsm-prefixed
> +nodes will appear in sysfs.
> +
> +Then, the user enables IDE by writing to /sys/bus/pci/devices/0000:e1:00.0/tsm_dev_connect
> +and this is how PCIe encryption is enabled.
> +
> +To pass the device through, a modifined VMM is required.
> +
> +In the VM, the same tsm.ko loads. In addition to the host's setup, the VM wants
> +to receive the report and enable secure DMA or/and secure MMIO, via some VM<->HV
> +protocol (such as AMD GHCB). Once this is done, a VM can access validated MMIO
> +with the Cbit set and the device can DMA to encrypted memory.
> +
> +
> +References
> +==========
> +
> +[1] TEE Device Interface Security Protocol - TDISP - v2022-07-27
> +https://members.pcisig.com/wg/PCI-SIG/document/18268?downloadRevision=21500
> +[2] Security Protocol and Data Model (SPDM)
> +https://www.dmtf.org/sites/default/files/standards/documents/DSP0274_1.2.1.pdf
> diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
> index 87d142c1f932..67a9c9daf96d 100644
> --- a/drivers/virt/coco/Kconfig
> +++ b/drivers/virt/coco/Kconfig
> @@ -7,6 +7,17 @@ config TSM_REPORTS
>  	select CONFIGFS_FS
>  	tristate
>  
> +config TSM
> +	tristate "Platform support for TEE Device Interface Security Protocol (TDISP)"
> +	default m

No defaulting to m.  People get grumpy when this stuff turns up on their embedded
distros with no hardware support.

> +	depends on AMD_MEM_ENCRYPT
> +	select PCI_DOE
> +	select PCI_IDE
> +	help
> +	  Add a common place for user visible platform support for PCIe TDISP.
> +	  TEE Device Interface Security Protocol (TDISP) from PCI-SIG,
> +	  https://pcisig.com/tee-device-interface-security-protocol-tdisp
> +
>  source "drivers/virt/coco/efi_secret/Kconfig"
>  
>  source "drivers/virt/coco/sev-guest/Kconfig"