| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6813caa28ddd1_1229d629412@dwillia2-xfh.jf.intel.com.notmuch>
Date: Thu, 1 May 2025 12:25:22 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: Cedric Xing <cedric.xing@...el.com>, Dan Williams
<dan.j.williams@...el.com>, "Kirill A. Shutemov"
<kirill.shutemov@...ux.intel.com>, Dave Hansen <dave.hansen@...ux.intel.com>,
Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
Borislav Petkov <bp@...en8.de>, <x86@...nel.org>, "H. Peter Anvin"
<hpa@...or.com>
CC: <linux-kernel@...r.kernel.org>, <linux-coco@...ts.linux.dev>, "Dionna
Amalie Glaze" <dionnaglaze@...gle.com>, Guorui Yu
<guorui.yu@...ux.alibaba.com>, James Bottomley
<James.Bottomley@...senpartnership.com>, Dan Middleton
<dan.middleton@...ux.intel.com>, Mikko Ylinen <mikko.ylinen@...ux.intel.com>,
Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@...ux.intel.com>,
Cedric Xing <cedric.xing@...el.com>
Subject: Re: [PATCH v5 1/5] tsm-mr: Add TVM Measurement Register support
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@...el.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@...el.com>
> L: linux-coco@...ts.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@....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@...el.com>
Powered by blists - more mailing lists