Re: [PATCH v5 1/5] tsm-mr: Add TVM Measurement Register support

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

Cedric Xing wrote:
> Introduce new TSM Measurement helper library (tsm-mr) for TVM guest drivers
> to expose MRs (Measurement Registers) as sysfs attributes, with Crypto
> Agility support.
> 
> Add the following new APIs (see include/linux/tsm-mr.h for details):
> 
> - tsm_mr_create_attribute_group(): Take on input a `struct
>   tsm_measurements` instance, which includes one `struct
>   tsm_measurement_register` per MR with properties like `TSM_MR_F_READABLE`
>   and `TSM_MR_F_WRITABLE`, to determine the supported operations and create
>   the sysfs attributes accordingly. On success, return a `struct
>   attribute_group` instance that will typically be included by the guest
>   driver into `miscdevice.groups` before calling misc_register().
> 
> - tsm_mr_free_attribute_group(): Free the memory allocated to the attrubute
>   group returned by tsm_mr_create_attribute_group().
> 
> tsm_mr_create_attribute_group() creates one attribute for each MR, with
> names following this pattern:
> 
>         MRNAME[:HASH]
> 
> - MRNAME - Placeholder for the MR name, as specified by
>   `tsm_measurement_register.mr_name`.
> - :HASH - Optional suffix indicating the hash algorithm associated with
>   this MR, as specified by `tsm_measurement_register.mr_hash`.
> 
> Support Crypto Agility by allowing multiple definitions of the same MR
> (i.e., with the same `mr_name`) with distinct HASH algorithms.
> 
> NOTE: Crypto Agility, introduced in TPM 2.0, allows new hash algorithms to
> be introduced without breaking compatibility with applications using older
> algorithms. CC architectures may face the same challenge in the future,
> needing new hashes for security while retaining compatibility with older
> hashes, hence the need for Crypto Agility.
> 
> Signed-off-by: Cedric Xing <cedric.xing@intel.com>

Looking good, some minor fixups below.

> ---
>  Documentation/driver-api/coco/index.rst            |  12 +
>  .../driver-api/coco/measurement-registers.rst      |  12 +
>  Documentation/driver-api/index.rst                 |   1 +
>  MAINTAINERS                                        |   4 +-
>  drivers/virt/coco/Kconfig                          |   5 +
>  drivers/virt/coco/Makefile                         |   1 +
>  drivers/virt/coco/tsm-mr.c                         | 247 +++++++++++++++++++++
>  include/linux/tsm-mr.h                             |  94 ++++++++
>  include/trace/events/tsm_mr.h                      |  80 +++++++
>  9 files changed, 454 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/driver-api/coco/index.rst b/Documentation/driver-api/coco/index.rst
> new file mode 100644
> index 000000000000..af9f08ca0cfd
> --- /dev/null
> +++ b/Documentation/driver-api/coco/index.rst
> @@ -0,0 +1,12 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +======================
> +Confidential Computing
> +======================
> +
> +.. toctree::
> +   :maxdepth: 1
> +
> +   measurement-registers
> +
> +.. only::  subproject and html
> diff --git a/Documentation/driver-api/coco/measurement-registers.rst b/Documentation/driver-api/coco/measurement-registers.rst
> new file mode 100644
> index 000000000000..cef85945a9a7
> --- /dev/null
> +++ b/Documentation/driver-api/coco/measurement-registers.rst
> @@ -0,0 +1,12 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +.. include:: <isonum.txt>
> +
> +=====================
> +Measurement Registers
> +=====================
> +
> +.. kernel-doc:: include/linux/tsm-mr.h
> +   :internal:
> +
> +.. kernel-doc:: drivers/virt/coco/tsm-mr.c
> +   :export:
> diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst
> index 16e2c4ec3c01..3e2a270bd828 100644
> --- a/Documentation/driver-api/index.rst
> +++ b/Documentation/driver-api/index.rst
> @@ -81,6 +81,7 @@ Subsystem-specific APIs
>     acpi/index
>     backlight/lp855x-driver.rst
>     clk
> +   coco/index
>     console
>     crypto/index
>     dmaengine/index
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fa1e04e87d1d..1d1426af3a68 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -24616,8 +24616,8 @@ M:	Dan Williams <dan.j.williams@intel.com>
>  L:	linux-coco@lists.linux.dev
>  S:	Maintained
>  F:	Documentation/ABI/testing/configfs-tsm
> -F:	drivers/virt/coco/tsm.c
> -F:	include/linux/tsm.h
> +F:	drivers/virt/coco/tsm*.c
> +F:	include/linux/tsm*.h

Should also add Documentation/driver-api/coco/ here.

>  
>  TRUSTED SERVICES TEE DRIVER
>  M:	Balint Dobszay <balint.dobszay@arm.com>
> diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
> index ff869d883d95..737106d5dcbc 100644
> --- a/drivers/virt/coco/Kconfig
> +++ b/drivers/virt/coco/Kconfig
> @@ -7,6 +7,11 @@ config TSM_REPORTS
>  	select CONFIGFS_FS
>  	tristate
>  
> +config TSM_MEASUREMENTS
> +	select CRYPTO_HASH_INFO
> +	select CRYPTO
> +	bool
> +
>  source "drivers/virt/coco/efi_secret/Kconfig"
>  
>  source "drivers/virt/coco/pkvm-guest/Kconfig"
> diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
> index c3d07cfc087e..eb6ec5c1d2e1 100644
> --- a/drivers/virt/coco/Makefile
> +++ b/drivers/virt/coco/Makefile
> @@ -3,6 +3,7 @@
>  # Confidential computing related collateral
>  #
>  obj-$(CONFIG_TSM_REPORTS)	+= tsm.o
> +obj-$(CONFIG_TSM_MEASUREMENTS)	+= tsm-mr.o
>  obj-$(CONFIG_EFI_SECRET)	+= efi_secret/
>  obj-$(CONFIG_ARM_PKVM_GUEST)	+= pkvm-guest/
>  obj-$(CONFIG_SEV_GUEST)		+= sev-guest/
> diff --git a/drivers/virt/coco/tsm-mr.c b/drivers/virt/coco/tsm-mr.c
> new file mode 100644
> index 000000000000..adc32328018e
> --- /dev/null
> +++ b/drivers/virt/coco/tsm-mr.c
> @@ -0,0 +1,247 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2024-2025 Intel Corporation. All rights reserved. */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/tsm_mr.h>
> +
> +/*
> + * struct tm_context - contains everything necessary to implement sysfs
> + * attributes for MRs.
> + * @rwsem: protects the MR cache from concurrent access.
> + * @agrp: contains all MR attributes created by tsm_mr_create_attribute_group().
> + * @tm: input to tsm_mr_create_attribute_group() containing MR definitions/ops.
> + * @in_sync: %true if MR cache is up-to-date.
> + * @mrs: array of &struct bin_attribute, one for each MR.
> + *
> + * This internal structure contains everything needed to implement
> + * tm_digest_read() and tm_digest_write().
> + *
> + * Given tm->refresh() is potentially expensive, tm_digest_read() caches MR
> + * values and calls tm->refresh() only when necessary. Only live MRs (i.e., with
> + * %TSM_MR_F_LIVE set) can trigger tm->refresh(), while others are assumed to
> + * retain their values from the last tm->write(). @in_sync tracks if there have
> + * been tm->write() calls since the last tm->refresh(). That is, tm->refresh()
> + * will be called only when a live MR is being read and the cache is stale
> + * (@in_sync is %false).
> + *
> + * tm_digest_write() sets @in_sync to %false and calls tm->write(), whose
> + * semantics is arch and MR specific. Most (if not all) writable MRs support the
> + * extension semantics (i.e., tm->write() extends the input buffer into the MR).
> + */
> +struct tm_context {
> +	struct rw_semaphore rwsem;
> +	struct attribute_group agrp;
> +	const struct tsm_measurements *tm;
> +	bool in_sync;
> +	struct bin_attribute mrs[];
> +};
> +
> +static ssize_t tm_digest_read(struct file *filp, struct kobject *kobj,
> +			      const struct bin_attribute *attr, char *buffer,
> +			      loff_t off, size_t count)
> +{
> +	struct tm_context *ctx;
> +	const struct tsm_measurement_register *mr;
> +	int rc;
> +
> +	ctx = attr->private;
> +	rc = down_read_interruptible(&ctx->rwsem);
> +	if (rc)
> +		return rc;
> +
> +	mr = &ctx->tm->mrs[attr - ctx->mrs];
> +
> +	/*
> +	 * @ctx->in_sync indicates if the MR cache is stale. It is a global
> +	 * instead of a per-MR flag for simplicity, as most (if not all) archs
> +	 * allow reading all MRs in oneshot.
> +	 *
> +	 * ctx->refresh() is necessary only for LIVE MRs, while others retain
> +	 * their values from their respective last ctx->write().
> +	 */
> +	if ((mr->mr_flags & TSM_MR_F_LIVE) && !ctx->in_sync) {
> +		up_read(&ctx->rwsem);
> +
> +		rc = down_write_killable(&ctx->rwsem);
> +		if (rc)
> +			return rc;
> +
> +		if (!ctx->in_sync) {
> +			rc = ctx->tm->refresh(ctx->tm, mr);
> +			ctx->in_sync = !rc;
> +			trace_tsm_mr_refresh(mr, rc);
> +		}
> +
> +		downgrade_write(&ctx->rwsem);
> +	}
> +
> +	memcpy(buffer, mr->mr_value + off, count);
> +	trace_tsm_mr_read(mr);
> +
> +	up_read(&ctx->rwsem);
> +	return rc ?: count;
> +}
> +
> +static ssize_t tm_digest_write(struct file *filp, struct kobject *kobj,
> +			       const struct bin_attribute *attr, char *buffer,
> +			       loff_t off, size_t count)
> +{
> +	struct tm_context *ctx;
> +	const struct tsm_measurement_register *mr;
> +	ssize_t rc;
> +
> +	/* partial writes are not supported */
> +	if (off != 0 || count != attr->size)
> +		return -EINVAL;
> +
> +	ctx = attr->private;
> +	mr = &ctx->tm->mrs[attr - ctx->mrs];
> +	trace_tsm_mr_write(mr, buffer);

I would have expected the trace to be emitted after the write succeeds,
otherwise this tracepoint is "may have written" vs "write".

> +
> +	rc = down_write_killable(&ctx->rwsem);
> +	if (rc)
> +		return rc;
> +
> +	rc = ctx->tm->write(ctx->tm, mr, buffer);
> +
> +	/* mark MR cache stale */
> +	if (!rc)
> +		ctx->in_sync = false;

...also, putting the trace here means the trace event is known to be
ordered by the rwsem.

[..]

Other than the above you can add:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>