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

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

Jonathan Cameron wrote:
> 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.

Yeah, subsys is awkward because even though this device is a singleton,
it is not a 'bus'. I will switch to 'struct tsm_core_dev' unless someone
comes up with a better name.

> 
> 
> > +
> > +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.

So, I do not think it matters in the end because there is no expectation
that anything but __free() invokes this call, and @subsys will be NULL
after no_free_ptr(). In general I expect that __free() callbacks should
mirror the "skip free()" condition in their DEFINE_FREE()
implementation. In this case though, the usage is so small and obvious
that we can pre-elide that code and just drop the duplicate check.

Going forward though I think this is something that deserves
clarification in cleanup.h documentation and I would argue that
DEFINE_FREE() and the __free handler duplicate the "skip free()"
condition check.

...or otherwise clarify the complication introduced by mixing ERR_PTR()
with no_free_ptr().