Re: [PATCH 03/11] coco/tsm: Introduce a class device for TEE Security Managers

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

On Thu, 05 Dec 2024 14:23:33 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> A "TSM" is a platform component that provides an API for securely
> provisioning resources for a confidential guest (TVM) to consume. The
> name originates from the PCI specification for platform agent that
> carries out operations for PCIe TDISP (TEE Device Interface Security
> Protocol).
> 
> Instances of this class device are parented by a device representing the
> platform security capability like CONFIG_CRYPTO_DEV_CCP or
> CONFIG_INTEL_TDX_HOST.
> 
> This class device interface is a frontend to the aspects of a TSM and
> TEE I/O that are cross-architecture common. This includes mechanisms
> like enumerating available platform TEE I/O capabilities and
> provisioning connections between the platform TSM and device DSMs
> (Device Security Manager (TDISP)).
> 
> For now this is just the scaffolding for registering a TSM device sysfs
> interface.
> 
> Cc: Xiaoyao Li <xiaoyao.li@intel.com>
> Cc: Isaku Yamahata <isaku.yamahata@intel.com>
> Cc: Alexey Kardashevskiy <aik@amd.com>
> Cc: Yilun Xu <yilun.xu@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: John Allen <john.allen@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

A couple of generic comments inline.

> diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
> index 819a97e8ba99..14e7cf145d85 100644
> --- a/drivers/virt/coco/Kconfig
> +++ b/drivers/virt/coco/Kconfig
> @@ -14,3 +14,5 @@ source "drivers/virt/coco/tdx-guest/Kconfig"
>  source "drivers/virt/coco/arm-cca-guest/Kconfig"
>  
>  source "drivers/virt/coco/guest/Kconfig"
> +
> +source "drivers/virt/coco/host/Kconfig"
> diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
> index 885c9ef4e9fc..73f1b7bc5b11 100644
> --- a/drivers/virt/coco/Makefile
> +++ b/drivers/virt/coco/Makefile
> @@ -8,3 +8,4 @@ obj-$(CONFIG_SEV_GUEST)		+= sev-guest/
>  obj-$(CONFIG_INTEL_TDX_GUEST)	+= tdx-guest/
>  obj-$(CONFIG_ARM_CCA_GUEST)	+= arm-cca-guest/
>  obj-$(CONFIG_TSM_REPORTS)	+= guest/
> +obj-y				+= host/
> diff --git a/drivers/virt/coco/host/Kconfig b/drivers/virt/coco/host/Kconfig
> new file mode 100644
> index 000000000000..4fbc6ef34f12
> --- /dev/null
> +++ b/drivers/virt/coco/host/Kconfig
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# TSM (TEE Security Manager) Common infrastructure and host drivers
> +#
> +config TSM
> +	tristate
> diff --git a/drivers/virt/coco/host/Makefile b/drivers/virt/coco/host/Makefile
> new file mode 100644
> index 000000000000..be0aba6007cd
> --- /dev/null
> +++ b/drivers/virt/coco/host/Makefile
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# TSM (TEE Security Manager) Common infrastructure and host drivers
> +
> +obj-$(CONFIG_TSM) += tsm.o
> +tsm-y := tsm-core.o
> diff --git a/drivers/virt/coco/host/tsm-core.c b/drivers/virt/coco/host/tsm-core.c
> new file mode 100644
> index 000000000000..0ee738fc40ed
> --- /dev/null
> +++ b/drivers/virt/coco/host/tsm-core.c
> @@ -0,0 +1,113 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2024 Intel Corporation. All rights reserved. */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/tsm.h>
> +#include <linux/rwsem.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/cleanup.h>
> +
> +static DECLARE_RWSEM(tsm_core_rwsem);
> +static struct class *tsm_class;
> +static struct tsm_subsys {
> +	struct device dev;
> +} *tsm_subsys;

This naming seems a bit confusing. To me tsm_sybsys could be:
a) The subsystem itself.  So something we'd expect to remove only alongside
class destroy.
b) A subsystem of a tsm (confusing here in a subsystem for tsms). Expectation
   being that a given tsm would register more than one of these.
c) What I think it is which is, which is the device added to register with
   the tsm subsystem.  

Mind you I'm not immediately sure what a better naming is.
tsm_class_dev maybe?  Though that sounds like it should be a struct device.


> +
> +static struct tsm_subsys *
> +alloc_tsm_subsys(struct device *parent, const struct attribute_group **groups)
> +{
> +	struct tsm_subsys *subsys = kzalloc(sizeof(*subsys), GFP_KERNEL);
> +	struct device *dev;
> +
> +	if (!subsys)
> +		return ERR_PTR(-ENOMEM);
> +	dev = &subsys->dev;
> +	dev->parent = parent;
> +	dev->groups = groups;
> +	dev->class = tsm_class;
> +	device_initialize(dev);
> +	return subsys;
> +}
> +
> +static void put_tsm_subsys(struct tsm_subsys *subsys)
> +{
> +	if (!IS_ERR_OR_NULL(subsys))

If you are calling this with it as an error or null then
that smells like a bad bug we shouldn't paper over.
The only case I can think of is the define free you have
below which correctly has the same check.

> +		put_device(&subsys->dev);
> +}
> +
> +DEFINE_FREE(put_tsm_subsys, struct tsm_subsys *,
> +	    if (!IS_ERR_OR_NULL(_T)) put_tsm_subsys(_T))
> +struct tsm_subsys *tsm_register(struct device *parent,
> +				const struct attribute_group **groups)
> +{
> +	struct device *dev;
> +	int rc;
> +
> +	guard(rwsem_write)(&tsm_core_rwsem);
> +	if (tsm_subsys) {
> +		dev_warn(parent, "failed to register: %s already registered\n",
> +			 dev_name(tsm_subsys->dev.parent));
> +		return ERR_PTR(-EBUSY);
> +	}
> +
> +	struct tsm_subsys *subsys __free(put_tsm_subsys) =
> +		alloc_tsm_subsys(parent, groups);
> +	if (IS_ERR(subsys))
> +		return subsys;
> +
> +	dev = &subsys->dev;
> +	rc = dev_set_name(dev, "tsm0");
> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	rc = device_add(dev);
> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	tsm_subsys = no_free_ptr(subsys);
> +
> +	return tsm_subsys;
> +}
> +EXPORT_SYMBOL_GPL(tsm_register);
> +
> +void tsm_unregister(struct tsm_subsys *subsys)
> +{
> +	guard(rwsem_write)(&tsm_core_rwsem);
> +	if (!tsm_subsys || subsys != tsm_subsys) {
> +		pr_warn("failed to unregister, not currently registered\n");
> +		return;
> +	}
> +
> +	device_unregister(&subsys->dev);
> +	tsm_subsys = NULL;
> +}
> +EXPORT_SYMBOL_GPL(tsm_unregister);
> +
> +static void tsm_release(struct device *dev)
> +{
> +	struct tsm_subsys *subsys = container_of(dev, typeof(*subsys), dev);
> +
> +	kfree(subsys);
> +}
> +
> +static int __init tsm_init(void)
> +{
> +	tsm_class = class_create("tsm");
> +	if (IS_ERR(tsm_class))
> +		return PTR_ERR(tsm_class);
> +
> +	tsm_class->dev_release = tsm_release;
> +	return 0;
> +}
> +module_init(tsm_init)
> +
> +static void __exit tsm_exit(void)
> +{
> +	class_destroy(tsm_class);
> +}
> +module_exit(tsm_exit)
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("TEE Security Manager core");