This commit extends the TSM core with support for CC measurement registers
(MRs).
The newly added APIs are:
- `tsm_register_measurement(struct tsm_measurement *)`: This API allows a
CC guest driver to register a set of measurement registers with the TSM
core.
- `tsm_unregister_measurement(struct tsm_measurement *)`: This API enables
a CC guest driver to unregister a previously registered set of
measurement registers.
`struct tsm_measurement` has been defined to encapsulate the details of
CC-specific MRs. It includes an array of `struct tsm_measurement_register`s
and provides operations for reading and updating these registers. For a
comprehensive understanding of the structure and its usage, refer to the
detailed comments in `include/linux/tsm.h`.
Upon successful registration of a measurement provider, the TSM core
exposes the MRs through a directory tree in the sysfs filesystem. The root
of this tree is located at `/sys/kernel/tsm/MR_PROVIDER/`, where
`MR_PROVIDER` is the name of the measurement provider (as specified by
`struct tsm_measurement::name`). Each MR is made accessible as either a
file or a directory of the specified name (i.e.,
`tsm_measurement_register::mr_name`). In the former case, the file content
is the MR value; while in the latter case `HASH_ALG/digest` under the MR
directory contains the MR value, where `HASH_ALG` specifies the hash
algorithm (e.g., sha256, sha384, etc.) used by this MR.
*Crypto Agility* is supported as a set of independent MRs that share a
common name. These MRs will be merged into a single MR directory and each
will be represented by its respective `HASH_ALG/digest` file. Note that
`tsm_measurement_register::mr_hash` must be distinct or the behavior is
undefined.
Signed-off-by: Cedric Xing <cedric.xing@intel.com>
---
Documentation/ABI/testing/sysfs-kernel-tsm | 20 ++
MAINTAINERS | 2 +-
drivers/virt/coco/Kconfig | 3 +-
drivers/virt/coco/Makefile | 2 +
drivers/virt/coco/{tsm.c => tsm-core.c} | 6 +-
drivers/virt/coco/tsm-mr.c | 375 +++++++++++++++++++++++++++++
include/linux/tsm.h | 64 +++++
7 files changed, 469 insertions(+), 3 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-kernel-tsm b/Documentation/ABI/testing/sysfs-kernel-tsm
new file mode 100644
index 000000000000..99735cf4da5c
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-kernel-tsm
@@ -0,0 +1,20 @@
+What: /sys/kernel/tsm/<measurement_provider>/<register>
+Date: February 2025
+Contact: Cedric Xing <cedric.xing@intel.com>.
+Description:
+ This file contains the value of the measurement register
+ <register>. Depending on the CC architecture, this file may be
+ writable, in which case the value written will be the new value
+ of <register>. Each write must start at the beginning and be of
+ the same size as the file. Partial writes are not permitted.
+
+What: /sys/kernel/tsm/<measurement_provider>/<register>/<hash>/digest
+Date: February 2025
+Contact: Cedric Xing <cedric.xing@intel.com>.
+Description:
+ This file contains the value of the measurement register
+ <register>. Depending on the CC architecture, this file may be
+ writable, in which case any value written may be extended to
+ <register> using <hash>. Each write must start at the beginning
+ and be of the same size as the file. Partial writes are not
+ permitted.
diff --git a/MAINTAINERS b/MAINTAINERS
index 25c86f47353d..c129fccd3d5a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -24098,7 +24098,7 @@ 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: drivers/virt/coco/tsm*.c
F: include/linux/tsm.h
TRUSTED SERVICES TEE DRIVER
diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
index ff869d883d95..6f3c0831680b 100644
--- a/drivers/virt/coco/Kconfig
+++ b/drivers/virt/coco/Kconfig
@@ -5,7 +5,8 @@
config TSM_REPORTS
select CONFIGFS_FS
- tristate
+ select CRYPTO_HASH_INFO
+ tristate "Trusted Security Module (TSM) sysfs/configfs support"
source "drivers/virt/coco/efi_secret/Kconfig"
diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
index c3d07cfc087e..4b108d8df1bd 100644
--- a/drivers/virt/coco/Makefile
+++ b/drivers/virt/coco/Makefile
@@ -2,6 +2,8 @@
#
# Confidential computing related collateral
#
+tsm-y += tsm-core.o tsm-mr.o
+
obj-$(CONFIG_TSM_REPORTS) += tsm.o
obj-$(CONFIG_EFI_SECRET) += efi_secret/
obj-$(CONFIG_ARM_PKVM_GUEST) += pkvm-guest/
diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm-core.c
similarity index 99%
rename from drivers/virt/coco/tsm.c
rename to drivers/virt/coco/tsm-core.c
index 9432d4e303f1..ab5269db9c13 100644
--- a/drivers/virt/coco/tsm.c
+++ b/drivers/virt/coco/tsm-core.c
@@ -476,6 +476,9 @@ int tsm_unregister(const struct tsm_ops *ops)
}
EXPORT_SYMBOL_GPL(tsm_unregister);
+int tsm_mr_init(void);
+void tsm_mr_exit(void);
+
static struct config_group *tsm_report_group;
static int __init tsm_init(void)
@@ -497,12 +500,13 @@ static int __init tsm_init(void)
}
tsm_report_group = tsm;
- return 0;
+ return tsm_mr_init();
}
module_init(tsm_init);
static void __exit tsm_exit(void)
{
+ tsm_mr_exit();
configfs_unregister_default_group(tsm_report_group);
configfs_unregister_subsystem(&tsm_configfs);
}
diff --git a/drivers/virt/coco/tsm-mr.c b/drivers/virt/coco/tsm-mr.c
new file mode 100644
index 000000000000..8d26e952da6b
--- /dev/null
+++ b/drivers/virt/coco/tsm-mr.c
@@ -0,0 +1,375 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright(c) 2024-2025 Intel Corporation. All rights reserved. */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <crypto/hash.h>
+#include <crypto/hash_info.h>
+#include <linux/kobject.h>
+#include <linux/module.h>
+#include <linux/tsm.h>
+
+int tsm_mr_init(void);
+void tsm_mr_exit(void);
+
+enum tmr_dir_battr_index {
+ TMR_DIR_BA_DIGEST,
+ TMR_DIR_BA__COUNT,
+
+ TMR_DIR__ALGO_MAX = 4,
+};
+
+struct tmr_dir {
+ struct kobject kobj;
+ struct bin_attribute battrs[TMR_DIR__ALGO_MAX][TMR_DIR_BA__COUNT];
+ int algo;
+};
+
+struct tmr_provider {
+ struct kset kset;
+ struct rw_semaphore rwsem;
+ struct bin_attribute *mrfiles;
+ struct tsm_measurement *tmr;
+ bool in_sync;
+};
+
+static inline struct tmr_provider *tmr_mr_to_provider(const struct tsm_measurement_register *mr,
+ struct kobject *kobj)
+{
+ if (mr->mr_flags & TSM_MR_F_F)
+ return container_of(kobj, struct tmr_provider, kset.kobj);
+ else
+ return container_of(kobj->kset, struct tmr_provider, kset);
+}
+
+static inline int tmr_call_refresh(struct tmr_provider *pvd,
+ const struct tsm_measurement_register *mr)
+{
+ int rc;
+
+ rc = pvd->tmr->refresh(pvd->tmr, mr);
+ if (rc)
+ pr_warn("%s.refresh(%s) failed %d\n", kobject_name(&pvd->kset.kobj), mr->mr_name,
+ rc);
+ return rc;
+}
+
+static inline int tmr_call_extend(struct tmr_provider *pvd,
+ const struct tsm_measurement_register *mr, const u8 *data)
+{
+ int rc;
+
+ rc = pvd->tmr->extend(pvd->tmr, mr, data);
+ if (rc)
+ pr_warn("%s.extend(%s) failed %d\n", kobject_name(&pvd->kset.kobj), mr->mr_name,
+ rc);
+ return rc;
+}
+
+static ssize_t tmr_digest_read(struct file *filp, struct kobject *kobj, struct bin_attribute *attr,
+ char *page, loff_t off, size_t count)
+{
+ const struct tsm_measurement_register *mr;
+ struct tmr_provider *pvd;
+ int rc;
+
+ if (off < 0 || off > attr->size)
+ return -EINVAL;
+
+ count = min(count, attr->size - (size_t)off);
+ if (!count)
+ return count;
+
+ mr = (typeof(mr))attr->private;
+ pvd = tmr_mr_to_provider(mr, kobj);
+ rc = down_read_interruptible(&pvd->rwsem);
+ if (rc)
+ return rc;
+
+ if ((mr->mr_flags & TSM_MR_F_L) && !pvd->in_sync) {
+ up_read(&pvd->rwsem);
+
+ rc = down_write_killable(&pvd->rwsem);
+ if (rc)
+ return rc;
+
+ if (!pvd->in_sync) {
+ rc = tmr_call_refresh(pvd, mr);
+ pvd->in_sync = !rc;
+ }
+
+ downgrade_write(&pvd->rwsem);
+ }
+
+ if (!rc)
+ memcpy(page, mr->mr_value + off, count);
+
+ up_read(&pvd->rwsem);
+ return rc ?: count;
+}
+
+static ssize_t tmr_digest_write(struct file *filp, struct kobject *kobj, struct bin_attribute *attr,
+ char *page, loff_t off, size_t count)
+{
+ const struct tsm_measurement_register *mr;
+ struct tmr_provider *pvd;
+ ssize_t rc;
+
+ if (off != 0 || count != attr->size)
+ return -EINVAL;
+
+ mr = (typeof(mr))attr->private;
+ pvd = tmr_mr_to_provider(mr, kobj);
+ rc = down_write_killable(&pvd->rwsem);
+ if (rc)
+ return rc;
+
+ if (mr->mr_flags & TSM_MR_F_X)
+ rc = tmr_call_extend(pvd, mr, page);
+ else
+ memcpy(mr->mr_value, page, count);
+
+ if (!rc)
+ pvd->in_sync = false;
+
+ up_write(&pvd->rwsem);
+ return rc ?: count;
+}
+
+static void tmr_dir_release(struct kobject *kobj)
+{
+ struct tmr_dir *mrd;
+
+ mrd = container_of(kobj, typeof(*mrd), kobj);
+ kfree(mrd);
+}
+
+static const struct kobj_type tmr_dir_ktype = {
+ .release = tmr_dir_release,
+ .sysfs_ops = &kobj_sysfs_ops,
+};
+
+static struct tmr_dir *tmr_dir_create(const struct tsm_measurement_register *mr,
+ struct tmr_provider *pvd)
+{
+ struct kobject *kobj;
+ struct tmr_dir *mrd;
+
+ kobj = kset_find_obj(&pvd->kset, mr->mr_name);
+ if (kobj) {
+ mrd = container_of(kobj, typeof(*mrd), kobj);
+ kobject_put(kobj);
+ if (++mrd->algo >= TMR_DIR__ALGO_MAX) {
+ --mrd->algo;
+ return ERR_PTR(-ENOSPC);
+ }
+ } else {
+ int rc;
+
+ mrd = kzalloc(sizeof(*mrd), GFP_KERNEL);
+ if (!mrd)
+ return ERR_PTR(-ENOMEM);
+
+ mrd->kobj.kset = &pvd->kset;
+ rc = kobject_init_and_add(&mrd->kobj, &tmr_dir_ktype, NULL, "%s", mr->mr_name);
+ if (rc) {
+ kfree(mrd);
+ return ERR_PTR(rc);
+ }
+ }
+
+ sysfs_bin_attr_init(&mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST]);
+ mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST].attr.name = "digest";
+ if (mr->mr_flags & TSM_MR_F_W)
+ mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST].attr.mode |= S_IWUSR | S_IWGRP;
+ if (mr->mr_flags & TSM_MR_F_R)
+ mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST].attr.mode |= S_IRUGO;
+
+ mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST].size = mr->mr_size;
+ mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST].read = tmr_digest_read;
+ mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST].write = tmr_digest_write;
+ mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST].private = (void *)mr;
+
+ return mrd;
+}
+
+static void tmr_provider_release(struct kobject *kobj)
+{
+ struct tmr_provider *pvd;
+
+ pvd = container_of(kobj, typeof(*pvd), kset.kobj);
+ if (!WARN_ON(!list_empty(&pvd->kset.list))) {
+ kfree(pvd->mrfiles);
+ kfree(pvd);
+ }
+}
+
+static const struct kobj_type _mr_provider_ktype = {
+ .release = tmr_provider_release,
+ .sysfs_ops = &kobj_sysfs_ops,
+};
+
+static struct kset *tmr_sysfs_root;
+
+static struct tmr_provider *tmr_provider_create(struct tsm_measurement *tmr)
+{
+ struct tmr_provider *pvd __free(kfree);
+ int rc;
+
+ pvd = kzalloc(sizeof(*pvd), GFP_KERNEL);
+ if (!pvd)
+ return ERR_PTR(-ENOMEM);
+
+ if (!tmr->name || !tmr->mrs || !tmr->refresh || !tmr->extend)
+ return ERR_PTR(-EINVAL);
+
+ rc = kobject_set_name(&pvd->kset.kobj, "%s", tmr->name);
+ if (rc)
+ return ERR_PTR(rc);
+
+ pvd->kset.kobj.kset = tmr_sysfs_root;
+ pvd->kset.kobj.ktype = &_mr_provider_ktype;
+ pvd->tmr = tmr;
+
+ init_rwsem(&pvd->rwsem);
+
+ rc = kset_register(&pvd->kset);
+ if (rc)
+ return ERR_PTR(rc);
+
+ return_ptr(pvd);
+}
+
+DEFINE_FREE(_unregister_measurement, struct tmr_provider *,
+ if (!IS_ERR_OR_NULL(_T)) tsm_unregister_measurement(_T->tmr));
+
+int tsm_register_measurement(struct tsm_measurement *tmr)
+{
+ struct tmr_provider *pvd __free(_unregister_measurement);
+ int rc, nr;
+
+ pvd = tmr_provider_create(tmr);
+ if (IS_ERR(pvd))
+ return PTR_ERR(pvd);
+
+ nr = 0;
+ for (int i = 0; tmr->mrs[i].mr_name; ++i) {
+ // flat files are counted and skipped
+ if (tmr->mrs[i].mr_flags & TSM_MR_F_F) {
+ ++nr;
+ continue;
+ }
+
+ struct tmr_dir *mrd;
+ struct bin_attribute *battrs[TMR_DIR_BA__COUNT + 1] = {};
+ struct attribute_group agrp = {
+ .name = hash_algo_name[tmr->mrs[i].mr_hash],
+ .bin_attrs = battrs,
+ };
+
+ mrd = tmr_dir_create(&tmr->mrs[i], pvd);
+ if (IS_ERR(mrd))
+ return PTR_ERR(mrd);
+
+ for (int j = 0; j < TMR_DIR_BA__COUNT; ++j)
+ battrs[j] = &mrd->battrs[mrd->algo][j];
+
+ rc = sysfs_create_group(&mrd->kobj, &agrp);
+ if (rc)
+ return rc;
+ }
+
+ if (nr > 0) {
+ struct bin_attribute *mrfiles __free(kfree);
+ struct bin_attribute **battrs __free(kfree);
+
+ mrfiles = kcalloc(nr, sizeof(*mrfiles), GFP_KERNEL);
+ battrs = kcalloc(nr + 1, sizeof(*battrs), GFP_KERNEL);
+ if (!battrs || !mrfiles)
+ return -ENOMEM;
+
+ for (int i = 0, j = 0; tmr->mrs[i].mr_name; ++i) {
+ if (!(tmr->mrs[i].mr_flags & TSM_MR_F_F))
+ continue;
+
+ mrfiles[j].attr.name = tmr->mrs[i].mr_name;
+ mrfiles[j].read = tmr_digest_read;
+ mrfiles[j].write = tmr_digest_write;
+ mrfiles[j].size = tmr->mrs[i].mr_size;
+ mrfiles[j].private = (void *)&tmr->mrs[i];
+ if (tmr->mrs[i].mr_flags & TSM_MR_F_R)
+ mrfiles[j].attr.mode |= S_IRUGO;
+ if (tmr->mrs[i].mr_flags & TSM_MR_F_W)
+ mrfiles[j].attr.mode |= S_IWUSR | S_IWGRP;
+
+ battrs[j] = &mrfiles[j];
+ ++j;
+ }
+
+ struct attribute_group agrp = {
+ .bin_attrs = battrs,
+ };
+ rc = sysfs_create_group(&pvd->kset.kobj, &agrp);
+ if (rc)
+ return rc;
+
+ pvd->mrfiles = no_free_ptr(mrfiles);
+ }
+
+ // initial refresh of MRs
+ rc = tmr_call_refresh(pvd, NULL);
+ pvd->in_sync = !rc;
+
+ pvd = NULL; // to avoid being freed automatically
+ return 0;
+}
+EXPORT_SYMBOL_GPL(tsm_register_measurement);
+
+static void tmr_put_children(struct kset *kset)
+{
+ struct kobject *p, *n;
+
+ spin_lock(&kset->list_lock);
+ list_for_each_entry_safe(p, n, &kset->list, entry) {
+ spin_unlock(&kset->list_lock);
+ kobject_put(p);
+ spin_lock(&kset->list_lock);
+ }
+ spin_unlock(&kset->list_lock);
+}
+
+int tsm_unregister_measurement(struct tsm_measurement *tmr)
+{
+ struct kobject *kobj;
+ struct tmr_provider *pvd;
+
+ kobj = kset_find_obj(tmr_sysfs_root, tmr->name);
+ if (!kobj)
+ return -ENOENT;
+
+ pvd = container_of(kobj, typeof(*pvd), kset.kobj);
+ if (pvd->tmr != tmr)
+ return -EINVAL;
+
+ tmr_put_children(&pvd->kset);
+ kset_unregister(&pvd->kset);
+ kobject_put(kobj);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(tsm_unregister_measurement);
+
+int tsm_mr_init(void)
+{
+ tmr_sysfs_root = kset_create_and_add("tsm", NULL, kernel_kobj);
+ if (!tmr_sysfs_root)
+ return -ENOMEM;
+ return 0;
+}
+
+void tsm_mr_exit(void)
+{
+ kset_unregister(tmr_sysfs_root);
+}
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Provide Trusted Security Module measurements via sysfs");
diff --git a/include/linux/tsm.h b/include/linux/tsm.h
index 11b0c525be30..624a7b62b85d 100644
--- a/include/linux/tsm.h
+++ b/include/linux/tsm.h
@@ -5,6 +5,7 @@
#include <linux/sizes.h>
#include <linux/types.h>
#include <linux/uuid.h>
+#include <uapi/linux/hash_info.h>
#define TSM_INBLOB_MAX 64
#define TSM_OUTBLOB_MAX SZ_32K
@@ -109,4 +110,67 @@ struct tsm_ops {
int tsm_register(const struct tsm_ops *ops, void *priv);
int tsm_unregister(const struct tsm_ops *ops);
+
+/**
+ * struct tsm_measurement_register - describes an architectural measurement register (MR)
+ * @mr_name: name of the MR
+ * @mr_value: buffer containing the current value of the MR
+ * @mr_size: size of the MR - typically the digest size of @mr_hash
+ * @mr_flags: bitwise OR of flags defined in enum tsm_measurement_register_flag
+ * @mr_hash: optional hash identifier defined in include/uapi/linux/hash_info.h
+ *
+ * A CC guest driver provides this structure to detail the measurement facility supported by the
+ * underlying CC hardware. After registration via `tsm_register_measurement`, the CC guest driver
+ * must retain this structure until it is unregistered using `tsm_unregister_measurement`.
+ */
+struct tsm_measurement_register {
+ const char *mr_name;
+ void *mr_value;
+ u32 mr_size;
+ u32 mr_flags;
+ enum hash_algo mr_hash;
+};
+
+/**
+ * enum tsm_measurement_register_flag - properties of an MR
+ * @TSM_MR_F_X: this MR supports the extension semantics on write
+ * @TSM_MR_F_W: this MR is writable
+ * @TSM_MR_F_R: this MR is readable. This should typically be set
+ * @TSM_MR_F_L: this MR is live - writes to other MRs may change this MR
+ * @TSM_MR_F_F: present this MR as a file (instead of a directory)
+ * @TSM_MR_F_LIVE: shorthand for L (live) and R (readable)
+ * @TSM_MR_F_RTMR: shorthand for LIVE and X (extensible)
+ */
+enum tsm_measurement_register_flag {
+ TSM_MR_F_X = 1,
+ TSM_MR_F_W = 2,
+ TSM_MR_F_R = 4,
+ TSM_MR_F_L = 8,
+ TSM_MR_F_F = 16,
+ TSM_MR_F_LIVE = TSM_MR_F_L | TSM_MR_F_R,
+ TSM_MR_F_RTMR = TSM_MR_F_LIVE | TSM_MR_F_X,
+};
+
+#define TSM_MR_(mr, hash) \
+ .mr_name = #mr, .mr_size = hash##_DIGEST_SIZE, .mr_hash = HASH_ALGO_##hash, \
+ .mr_flags = TSM_MR_F_R
+
+/**
+ * struct tsm_measurement - define CC specific MRs and methods for updating them
+ * @name: name of the measurement provider
+ * @mrs: array of MR definitions ending with mr_name set to %NULL
+ * @refresh: invoked to update the specified MR
+ * @extend: invoked to extend the specified MR with mr_size bytes
+ */
+struct tsm_measurement {
+ const char *name;
+ const struct tsm_measurement_register *mrs;
+ int (*refresh)(struct tsm_measurement *tmr, const struct tsm_measurement_register *mr);
+ int (*extend)(struct tsm_measurement *tmr, const struct tsm_measurement_register *mr,
+ const u8 *data);
+};
+
+int tsm_register_measurement(struct tsm_measurement *tmr);
+int tsm_unregister_measurement(struct tsm_measurement *tmr);
+
#endif /* __TSM_H */
--
2.43.0Hi Cedric,
On 2/12/25 6:23 PM, Cedric Xing wrote:
> This commit extends the TSM core with support for CC measurement registers
> (MRs).
>
> The newly added APIs are:
>
> - `tsm_register_measurement(struct tsm_measurement *)`: This API allows a
> CC guest driver to register a set of measurement registers with the TSM
> core.
> - `tsm_unregister_measurement(struct tsm_measurement *)`: This API enables
> a CC guest driver to unregister a previously registered set of
> measurement registers.
>
> `struct tsm_measurement` has been defined to encapsulate the details of
> CC-specific MRs. It includes an array of `struct tsm_measurement_register`s
> and provides operations for reading and updating these registers. For a
> comprehensive understanding of the structure and its usage, refer to the
> detailed comments in `include/linux/tsm.h`.
>
> Upon successful registration of a measurement provider, the TSM core
> exposes the MRs through a directory tree in the sysfs filesystem. The root
> of this tree is located at `/sys/kernel/tsm/MR_PROVIDER/`, where
> `MR_PROVIDER` is the name of the measurement provider (as specified by
> `struct tsm_measurement::name`). Each MR is made accessible as either a
> file or a directory of the specified name (i.e.,
> `tsm_measurement_register::mr_name`). In the former case, the file content
May be include some info on when a MR can be just a file (like an example)
> is the MR value; while in the latter case `HASH_ALG/digest` under the MR
> directory contains the MR value, where `HASH_ALG` specifies the hash
> algorithm (e.g., sha256, sha384, etc.) used by this MR.
>
> *Crypto Agility* is supported as a set of independent MRs that share a
> common name. These MRs will be merged into a single MR directory and each
> will be represented by its respective `HASH_ALG/digest` file. Note that
> `tsm_measurement_register::mr_hash` must be distinct or the behavior is
> undefined.
is this required/supported in any of the existing CC providers?
By sharing a common name, you mean internally there will be distinct
registers for every crypto algo supported?
>
> Signed-off-by: Cedric Xing <cedric.xing@intel.com>
> ---
> Documentation/ABI/testing/sysfs-kernel-tsm | 20 ++
> MAINTAINERS | 2 +-
> drivers/virt/coco/Kconfig | 3 +-
> drivers/virt/coco/Makefile | 2 +
> drivers/virt/coco/{tsm.c => tsm-core.c} | 6 +-
> drivers/virt/coco/tsm-mr.c | 375 +++++++++++++++++++++++++++++
> include/linux/tsm.h | 64 +++++
> 7 files changed, 469 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-kernel-tsm b/Documentation/ABI/testing/sysfs-kernel-tsm
> new file mode 100644
> index 000000000000..99735cf4da5c
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-kernel-tsm
> @@ -0,0 +1,20 @@
> +What: /sys/kernel/tsm/<measurement_provider>/<register>
Any reason for not using fixed name for registers (like mr[0-n])? May be it
will help if user space use a generic code across vendors.
> +Date: February 2025
> +Contact: Cedric Xing <cedric.xing@intel.com>.
> +Description:
> + This file contains the value of the measurement register
> + <register>. Depending on the CC architecture, this file may be
> + writable, in which case the value written will be the new value
> + of <register>. Each write must start at the beginning and be of
> + the same size as the file. Partial writes are not permitted.
> +
> +What: /sys/kernel/tsm/<measurement_provider>/<register>/<hash>/digest
> +Date: February 2025
> +Contact: Cedric Xing <cedric.xing@intel.com>.
> +Description:
> + This file contains the value of the measurement register
> + <register>. Depending on the CC architecture, this file may be
> + writable, in which case any value written may be extended to
> + <register> using <hash>. Each write must start at the beginning
> + and be of the same size as the file. Partial writes are not
> + permitted.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 25c86f47353d..c129fccd3d5a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -24098,7 +24098,7 @@ 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: drivers/virt/coco/tsm*.c
> F: include/linux/tsm.h
>
> TRUSTED SERVICES TEE DRIVER
> diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
> index ff869d883d95..6f3c0831680b 100644
> --- a/drivers/virt/coco/Kconfig
> +++ b/drivers/virt/coco/Kconfig
> @@ -5,7 +5,8 @@
>
> config TSM_REPORTS
> select CONFIGFS_FS
> - tristate
> + select CRYPTO_HASH_INFO
> + tristate "Trusted Security Module (TSM) sysfs/configfs support"
IMO, sysfs/configfs part is not required in the title.
>
> source "drivers/virt/coco/efi_secret/Kconfig"
>
> diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
> index c3d07cfc087e..4b108d8df1bd 100644
> --- a/drivers/virt/coco/Makefile
> +++ b/drivers/virt/coco/Makefile
> @@ -2,6 +2,8 @@
> #
> # Confidential computing related collateral
> #
> +tsm-y += tsm-core.o tsm-mr.o
> +
> obj-$(CONFIG_TSM_REPORTS) += tsm.o
> obj-$(CONFIG_EFI_SECRET) += efi_secret/
> obj-$(CONFIG_ARM_PKVM_GUEST) += pkvm-guest/
> diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm-core.c
> similarity index 99%
> rename from drivers/virt/coco/tsm.c
> rename to drivers/virt/coco/tsm-core.c
> index 9432d4e303f1..ab5269db9c13 100644
> --- a/drivers/virt/coco/tsm.c
> +++ b/drivers/virt/coco/tsm-core.c
> @@ -476,6 +476,9 @@ int tsm_unregister(const struct tsm_ops *ops)
> }
> EXPORT_SYMBOL_GPL(tsm_unregister);
>
> +int tsm_mr_init(void);
> +void tsm_mr_exit(void);
> +
> static struct config_group *tsm_report_group;
>
> static int __init tsm_init(void)
> @@ -497,12 +500,13 @@ static int __init tsm_init(void)
> }
> tsm_report_group = tsm;
>
> - return 0;
> + return tsm_mr_init();
> }
> module_init(tsm_init);
>
> static void __exit tsm_exit(void)
> {
> + tsm_mr_exit();
> configfs_unregister_default_group(tsm_report_group);
> configfs_unregister_subsystem(&tsm_configfs);
> }
> diff --git a/drivers/virt/coco/tsm-mr.c b/drivers/virt/coco/tsm-mr.c
> new file mode 100644
> index 000000000000..8d26e952da6b
> --- /dev/null
> +++ b/drivers/virt/coco/tsm-mr.c
> @@ -0,0 +1,375 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2024-2025 Intel Corporation. All rights reserved. */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <crypto/hash.h>
> +#include <crypto/hash_info.h>
> +#include <linux/kobject.h>
> +#include <linux/module.h>
> +#include <linux/tsm.h>
> +
> +int tsm_mr_init(void);
> +void tsm_mr_exit(void);
> +
> +enum tmr_dir_battr_index {
> + TMR_DIR_BA_DIGEST,
> + TMR_DIR_BA__COUNT,
Why not use single underscore uniformly?
> +
> + TMR_DIR__ALGO_MAX = 4,
Since this is not related to attribute index, why not use #define?
> +};
> +
> +struct tmr_dir {
> + struct kobject kobj;
> + struct bin_attribute battrs[TMR_DIR__ALGO_MAX][TMR_DIR_BA__COUNT];
> + int algo;
> +};
> +
> +struct tmr_provider {
> + struct kset kset;
> + struct rw_semaphore rwsem;
> + struct bin_attribute *mrfiles;
> + struct tsm_measurement *tmr;
> + bool in_sync;
> +};
> +
> +static inline struct tmr_provider *tmr_mr_to_provider(const struct tsm_measurement_register *mr,
> + struct kobject *kobj)
> +{
> + if (mr->mr_flags & TSM_MR_F_F)
> + return container_of(kobj, struct tmr_provider, kset.kobj);
> + else
> + return container_of(kobj->kset, struct tmr_provider, kset);
> +}
> +
> +static inline int tmr_call_refresh(struct tmr_provider *pvd,
> + const struct tsm_measurement_register *mr)
> +{
> + int rc;
> +
> + rc = pvd->tmr->refresh(pvd->tmr, mr);
> + if (rc)
> + pr_warn("%s.refresh(%s) failed %d\n", kobject_name(&pvd->kset.kobj), mr->mr_name,
> + rc);
> + return rc;
> +}
> +
> +static inline int tmr_call_extend(struct tmr_provider *pvd,
> + const struct tsm_measurement_register *mr, const u8 *data)
> +{
> + int rc;
> +
> + rc = pvd->tmr->extend(pvd->tmr, mr, data);
> + if (rc)
> + pr_warn("%s.extend(%s) failed %d\n", kobject_name(&pvd->kset.kobj), mr->mr_name,
> + rc);
> + return rc;
> +}
> +
> +static ssize_t tmr_digest_read(struct file *filp, struct kobject *kobj, struct bin_attribute *attr,
> + char *page, loff_t off, size_t count)
> +{
> + const struct tsm_measurement_register *mr;
> + struct tmr_provider *pvd;
> + int rc;
> +
> + if (off < 0 || off > attr->size)
> + return -EINVAL;
> +
> + count = min(count, attr->size - (size_t)off);
> + if (!count)
> + return count;
> +
> + mr = (typeof(mr))attr->private;
I think you don't need to cast it.
> + pvd = tmr_mr_to_provider(mr, kobj);
> + rc = down_read_interruptible(&pvd->rwsem);
> + if (rc)
> + return rc;
> +
> + if ((mr->mr_flags & TSM_MR_F_L) && !pvd->in_sync) {
> + up_read(&pvd->rwsem);
> +
> + rc = down_write_killable(&pvd->rwsem);
> + if (rc)
> + return rc;
> +
> + if (!pvd->in_sync) {
Since this path is only taken if in_sync is false, do you need to check
again?
> + rc = tmr_call_refresh(pvd, mr);
> + pvd->in_sync = !rc;
> + }
> +
> + downgrade_write(&pvd->rwsem);
> + }
> +
> + if (!rc)
> + memcpy(page, mr->mr_value + off, count);
> +
> + up_read(&pvd->rwsem);
> + return rc ?: count;
> +}
> +
> +static ssize_t tmr_digest_write(struct file *filp, struct kobject *kobj, struct bin_attribute *attr,
> + char *page, loff_t off, size_t count)
> +{
> + const struct tsm_measurement_register *mr;
> + struct tmr_provider *pvd;
> + ssize_t rc;
> +
> + if (off != 0 || count != attr->size)
> + return -EINVAL;
> +
> + mr = (typeof(mr))attr->private;
> + pvd = tmr_mr_to_provider(mr, kobj);
> + rc = down_write_killable(&pvd->rwsem);
> + if (rc)
> + return rc;
> +
> + if (mr->mr_flags & TSM_MR_F_X)
> + rc = tmr_call_extend(pvd, mr, page);
> + else
> + memcpy(mr->mr_value, page, count);
> +
> + if (!rc)
> + pvd->in_sync = false;
> +
> + up_write(&pvd->rwsem);
> + return rc ?: count;
> +}
> +
> +static void tmr_dir_release(struct kobject *kobj)
> +{
> + struct tmr_dir *mrd;
> +
> + mrd = container_of(kobj, typeof(*mrd), kobj);
> + kfree(mrd);
> +}
> +
> +static const struct kobj_type tmr_dir_ktype = {
> + .release = tmr_dir_release,
> + .sysfs_ops = &kobj_sysfs_ops,
> +};
> +
> +static struct tmr_dir *tmr_dir_create(const struct tsm_measurement_register *mr,
> + struct tmr_provider *pvd)
> +{
> + struct kobject *kobj;
> + struct tmr_dir *mrd;
> +
> + kobj = kset_find_obj(&pvd->kset, mr->mr_name);
> + if (kobj) {
> + mrd = container_of(kobj, typeof(*mrd), kobj);
> + kobject_put(kobj);
> + if (++mrd->algo >= TMR_DIR__ALGO_MAX) {
> + --mrd->algo;
> + return ERR_PTR(-ENOSPC);
> + }
> + } else {
> + int rc;
> +
> + mrd = kzalloc(sizeof(*mrd), GFP_KERNEL);
> + if (!mrd)
> + return ERR_PTR(-ENOMEM);
> +
> + mrd->kobj.kset = &pvd->kset;
> + rc = kobject_init_and_add(&mrd->kobj, &tmr_dir_ktype, NULL, "%s", mr->mr_name);
> + if (rc) {
> + kfree(mrd);
> + return ERR_PTR(rc);
> + }
> + }
> +
> + sysfs_bin_attr_init(&mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST]);
> + mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST].attr.name = "digest";
Since this attribute reflects register value, personally I think "value"
is more clear
than "digest". But it is fine either way.
> + if (mr->mr_flags & TSM_MR_F_W)
> + mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST].attr.mode |= S_IWUSR | S_IWGRP;
> + if (mr->mr_flags & TSM_MR_F_R)
> + mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST].attr.mode |= S_IRUGO;
> +
> + mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST].size = mr->mr_size;
> + mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST].read = tmr_digest_read;
> + mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST].write = tmr_digest_write;
> + mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST].private = (void *)mr;
> +
> + return mrd;
> +}
> +
> +static void tmr_provider_release(struct kobject *kobj)
> +{
> + struct tmr_provider *pvd;
> +
> + pvd = container_of(kobj, typeof(*pvd), kset.kobj);
> + if (!WARN_ON(!list_empty(&pvd->kset.list))) {
> + kfree(pvd->mrfiles);
> + kfree(pvd);
> + }
> +}
> +
> +static const struct kobj_type _mr_provider_ktype = {
> + .release = tmr_provider_release,
> + .sysfs_ops = &kobj_sysfs_ops,
> +};
> +
> +static struct kset *tmr_sysfs_root;
> +
> +static struct tmr_provider *tmr_provider_create(struct tsm_measurement *tmr)
> +{
> + struct tmr_provider *pvd __free(kfree);
> + int rc;
> +
> + pvd = kzalloc(sizeof(*pvd), GFP_KERNEL);
> + if (!pvd)
> + return ERR_PTR(-ENOMEM);
> +
> + if (!tmr->name || !tmr->mrs || !tmr->refresh || !tmr->extend)
> + return ERR_PTR(-EINVAL);
Why not add this condition at the top before allocation?
> +
> + rc = kobject_set_name(&pvd->kset.kobj, "%s", tmr->name);
> + if (rc)
> + return ERR_PTR(rc);
> +
> + pvd->kset.kobj.kset = tmr_sysfs_root;
> + pvd->kset.kobj.ktype = &_mr_provider_ktype;
> + pvd->tmr = tmr;
> +
> + init_rwsem(&pvd->rwsem);
> +
> + rc = kset_register(&pvd->kset);
> + if (rc)
> + return ERR_PTR(rc);
> +
> + return_ptr(pvd);
> +}
> +
> +DEFINE_FREE(_unregister_measurement, struct tmr_provider *,
> + if (!IS_ERR_OR_NULL(_T)) tsm_unregister_measurement(_T->tmr));
> +
> +int tsm_register_measurement(struct tsm_measurement *tmr)
> +{
> + struct tmr_provider *pvd __free(_unregister_measurement);
> + int rc, nr;
> +
> + pvd = tmr_provider_create(tmr);
> + if (IS_ERR(pvd))
> + return PTR_ERR(pvd);
> +
> + nr = 0;
> + for (int i = 0; tmr->mrs[i].mr_name; ++i) {
> + // flat files are counted and skipped
> + if (tmr->mrs[i].mr_flags & TSM_MR_F_F) {
> + ++nr;
> + continue;
> + }
> +
> + struct tmr_dir *mrd;
> + struct bin_attribute *battrs[TMR_DIR_BA__COUNT + 1] = {};
> + struct attribute_group agrp = {
> + .name = hash_algo_name[tmr->mrs[i].mr_hash],
> + .bin_attrs = battrs,
> + };
> +
> + mrd = tmr_dir_create(&tmr->mrs[i], pvd);
> + if (IS_ERR(mrd))
> + return PTR_ERR(mrd);
> +
> + for (int j = 0; j < TMR_DIR_BA__COUNT; ++j)
> + battrs[j] = &mrd->battrs[mrd->algo][j];
> +
> + rc = sysfs_create_group(&mrd->kobj, &agrp);
> + if (rc)
> + return rc;
> + }
> +
> + if (nr > 0) {
> + struct bin_attribute *mrfiles __free(kfree);
> + struct bin_attribute **battrs __free(kfree);
> +
> + mrfiles = kcalloc(nr, sizeof(*mrfiles), GFP_KERNEL);
> + battrs = kcalloc(nr + 1, sizeof(*battrs), GFP_KERNEL);
> + if (!battrs || !mrfiles)
> + return -ENOMEM;
> +
> + for (int i = 0, j = 0; tmr->mrs[i].mr_name; ++i) {
> + if (!(tmr->mrs[i].mr_flags & TSM_MR_F_F))
> + continue;
> +
> + mrfiles[j].attr.name = tmr->mrs[i].mr_name;
> + mrfiles[j].read = tmr_digest_read;
> + mrfiles[j].write = tmr_digest_write;
> + mrfiles[j].size = tmr->mrs[i].mr_size;
> + mrfiles[j].private = (void *)&tmr->mrs[i];
> + if (tmr->mrs[i].mr_flags & TSM_MR_F_R)
> + mrfiles[j].attr.mode |= S_IRUGO;
> + if (tmr->mrs[i].mr_flags & TSM_MR_F_W)
> + mrfiles[j].attr.mode |= S_IWUSR | S_IWGRP;
> +
> + battrs[j] = &mrfiles[j];
> + ++j;
> + }
> +
> + struct attribute_group agrp = {
> + .bin_attrs = battrs,
> + };
> + rc = sysfs_create_group(&pvd->kset.kobj, &agrp);
> + if (rc)
> + return rc;
> +
> + pvd->mrfiles = no_free_ptr(mrfiles);
> + }
> +
> + // initial refresh of MRs
> + rc = tmr_call_refresh(pvd, NULL);
> + pvd->in_sync = !rc;
> +
> + pvd = NULL; // to avoid being freed automatically
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(tsm_register_measurement);
> +
> +static void tmr_put_children(struct kset *kset)
> +{
> + struct kobject *p, *n;
> +
> + spin_lock(&kset->list_lock);
> + list_for_each_entry_safe(p, n, &kset->list, entry) {
> + spin_unlock(&kset->list_lock);
> + kobject_put(p);
> + spin_lock(&kset->list_lock);
> + }
> + spin_unlock(&kset->list_lock);
> +}
> +
> +int tsm_unregister_measurement(struct tsm_measurement *tmr)
> +{
> + struct kobject *kobj;
> + struct tmr_provider *pvd;
> +
> + kobj = kset_find_obj(tmr_sysfs_root, tmr->name);
> + if (!kobj)
> + return -ENOENT;
> +
> + pvd = container_of(kobj, typeof(*pvd), kset.kobj);
> + if (pvd->tmr != tmr)
> + return -EINVAL;
> +
> + tmr_put_children(&pvd->kset);
> + kset_unregister(&pvd->kset);
> + kobject_put(kobj);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(tsm_unregister_measurement);
> +
> +int tsm_mr_init(void)
> +{
> + tmr_sysfs_root = kset_create_and_add("tsm", NULL, kernel_kobj);
> + if (!tmr_sysfs_root)
> + return -ENOMEM;
> + return 0;
> +}
> +
> +void tsm_mr_exit(void)
> +{
> + kset_unregister(tmr_sysfs_root);
> +}
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Provide Trusted Security Module measurements via sysfs");
> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> index 11b0c525be30..624a7b62b85d 100644
> --- a/include/linux/tsm.h
> +++ b/include/linux/tsm.h
> @@ -5,6 +5,7 @@
> #include <linux/sizes.h>
> #include <linux/types.h>
> #include <linux/uuid.h>
> +#include <uapi/linux/hash_info.h>
>
> #define TSM_INBLOB_MAX 64
> #define TSM_OUTBLOB_MAX SZ_32K
> @@ -109,4 +110,67 @@ struct tsm_ops {
>
> int tsm_register(const struct tsm_ops *ops, void *priv);
> int tsm_unregister(const struct tsm_ops *ops);
> +
> +/**
> + * struct tsm_measurement_register - describes an architectural measurement register (MR)
> + * @mr_name: name of the MR
> + * @mr_value: buffer containing the current value of the MR
> + * @mr_size: size of the MR - typically the digest size of @mr_hash
> + * @mr_flags: bitwise OR of flags defined in enum tsm_measurement_register_flag
> + * @mr_hash: optional hash identifier defined in include/uapi/linux/hash_info.h
> + *
> + * A CC guest driver provides this structure to detail the measurement facility supported by the
> + * underlying CC hardware. After registration via `tsm_register_measurement`, the CC guest driver
> + * must retain this structure until it is unregistered using `tsm_unregister_measurement`.
> + */
> +struct tsm_measurement_register {
> + const char *mr_name;
> + void *mr_value;
> + u32 mr_size;
> + u32 mr_flags;
> + enum hash_algo mr_hash;
> +};
> +
> +/**
> + * enum tsm_measurement_register_flag - properties of an MR
> + * @TSM_MR_F_X: this MR supports the extension semantics on write
Why not use _E? Before reading the help text, I thought _X is for execute.
> + * @TSM_MR_F_W: this MR is writable
> + * @TSM_MR_F_R: this MR is readable. This should typically be set
> + * @TSM_MR_F_L: this MR is live - writes to other MRs may change this MR
> + * @TSM_MR_F_F: present this MR as a file (instead of a directory)
> + * @TSM_MR_F_LIVE: shorthand for L (live) and R (readable)
> + * @TSM_MR_F_RTMR: shorthand for LIVE and X (extensible)
> + */
> +enum tsm_measurement_register_flag {
> + TSM_MR_F_X = 1,
> + TSM_MR_F_W = 2,
It is not clear why you want to differentiate between write and extension.
Please add some help text related to it.
> + TSM_MR_F_R = 4,
> + TSM_MR_F_L = 8,
> + TSM_MR_F_F = 16,
> + TSM_MR_F_LIVE = TSM_MR_F_L | TSM_MR_F_R,
> + TSM_MR_F_RTMR = TSM_MR_F_LIVE | TSM_MR_F_X,
> +};
> +
> +#define TSM_MR_(mr, hash) \
> + .mr_name = #mr, .mr_size = hash##_DIGEST_SIZE, .mr_hash = HASH_ALGO_##hash, \
> + .mr_flags = TSM_MR_F_R
> +
> +/**
> + * struct tsm_measurement - define CC specific MRs and methods for updating them
> + * @name: name of the measurement provider
> + * @mrs: array of MR definitions ending with mr_name set to %NULL
> + * @refresh: invoked to update the specified MR
> + * @extend: invoked to extend the specified MR with mr_size bytes
> + */
> +struct tsm_measurement {
> + const char *name;
> + const struct tsm_measurement_register *mrs;
> + int (*refresh)(struct tsm_measurement *tmr, const struct tsm_measurement_register *mr);
> + int (*extend)(struct tsm_measurement *tmr, const struct tsm_measurement_register *mr,
> + const u8 *data);
> +};
> +
> +int tsm_register_measurement(struct tsm_measurement *tmr);
> +int tsm_unregister_measurement(struct tsm_measurement *tmr);
> +
> #endif /* __TSM_H */
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On 2/17/2025 7:10 PM, Sathyanarayanan Kuppuswamy wrote:
> Hi Cedric,
>
> On 2/12/25 6:23 PM, Cedric Xing wrote:
>> This commit extends the TSM core with support for CC measurement
>> registers
>> (MRs).
>>
>> The newly added APIs are:
>>
>> - `tsm_register_measurement(struct tsm_measurement *)`: This API allows a
>> CC guest driver to register a set of measurement registers with the
>> TSM
>> core.
>> - `tsm_unregister_measurement(struct tsm_measurement *)`: This API
>> enables
>> a CC guest driver to unregister a previously registered set of
>> measurement registers.
>>
>> `struct tsm_measurement` has been defined to encapsulate the details of
>> CC-specific MRs. It includes an array of `struct
>> tsm_measurement_register`s
>> and provides operations for reading and updating these registers. For a
>> comprehensive understanding of the structure and its usage, refer to the
>> detailed comments in `include/linux/tsm.h`.
>>
>> Upon successful registration of a measurement provider, the TSM core
>> exposes the MRs through a directory tree in the sysfs filesystem. The
>> root
>> of this tree is located at `/sys/kernel/tsm/MR_PROVIDER/`, where
>> `MR_PROVIDER` is the name of the measurement provider (as specified by
>> `struct tsm_measurement::name`). Each MR is made accessible as either a
>> file or a directory of the specified name (i.e.,
>> `tsm_measurement_register::mr_name`). In the former case, the file
>> content
>
> May be include some info on when a MR can be just a file (like an example)
>
Will do.
>> is the MR value; while in the latter case `HASH_ALG/digest` under the MR
>> directory contains the MR value, where `HASH_ALG` specifies the hash
>> algorithm (e.g., sha256, sha384, etc.) used by this MR.
>>
>> *Crypto Agility* is supported as a set of independent MRs that share a
>> common name. These MRs will be merged into a single MR directory and each
>> will be represented by its respective `HASH_ALG/digest` file. Note that
>> `tsm_measurement_register::mr_hash` must be distinct or the behavior is
>> undefined.
>
> is this required/supported in any of the existing CC providers?
>
> By sharing a common name, you mean internally there will be distinct
> registers for every crypto algo supported?
>
This is explicitly requested by James Bottomley. And yes, an MR with >1
algo is in fact a collection of independent MRs, even though they are
referred to as the "banks" of the same MR in TCG/TPM spec.
>>
>> Signed-off-by: Cedric Xing <cedric.xing@intel.com>
>> ---
>> Documentation/ABI/testing/sysfs-kernel-tsm | 20 ++
>> MAINTAINERS | 2 +-
>> drivers/virt/coco/Kconfig | 3 +-
>> drivers/virt/coco/Makefile | 2 +
>> drivers/virt/coco/{tsm.c => tsm-core.c} | 6 +-
>> drivers/virt/coco/tsm-mr.c | 375 +++++++++++++++++++
>> ++++++++++
>> include/linux/tsm.h | 64 +++++
>> 7 files changed, 469 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-kernel-tsm b/
>> Documentation/ABI/testing/sysfs-kernel-tsm
>> new file mode 100644
>> index 000000000000..99735cf4da5c
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-kernel-tsm
>> @@ -0,0 +1,20 @@
>> +What: /sys/kernel/tsm/<measurement_provider>/<register>
>
> Any reason for not using fixed name for registers (like mr[0-n])? May be it
> will help if user space use a generic code across vendors.
>
The names of MRs identify the hardware resources, while the semantics of
an MR (e.g., whose measurements it contains) is defined by applications.
A good design should separate those two to allow applications to connect
them as needed. For example, say all loadable modules must be measured
before being loaded, and shall be measured/extended to rtmrX on arch A
and to rtmrY on arch B, respectively. A portable implementation could
always extend to "rtmrModule", which would be a symlink pointing to
either rtmrX or rtmrY depending on the underlying arch. On the contrary,
extending to the same mr[z] is equivalent to forcing z == X == Y, which
_breaks_ portability as different archs tend to have different number of
RTMRs.
One more thing worth noting here is: different archs may choose
different hash algorithms for RTMRs, and that forces applications to be
arch aware. The solution will be a "log centric" ABI that we don't have yet.
>> +Date: February 2025
>> +Contact: Cedric Xing <cedric.xing@intel.com>.
>> +Description:
>> + This file contains the value of the measurement register
>> + <register>. Depending on the CC architecture, this file may be
>> + writable, in which case the value written will be the new value
>> + of <register>. Each write must start at the beginning and be of
>> + the same size as the file. Partial writes are not permitted.
>> +
>> +What: /sys/kernel/tsm/<measurement_provider>/<register>/
>> <hash>/digest
>> +Date: February 2025
>> +Contact: Cedric Xing <cedric.xing@intel.com>.
>> +Description:
>> + This file contains the value of the measurement register
>> + <register>. Depending on the CC architecture, this file may be
>> + writable, in which case any value written may be extended to
>> + <register> using <hash>. Each write must start at the beginning
>> + and be of the same size as the file. Partial writes are not
>> + permitted.
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 25c86f47353d..c129fccd3d5a 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -24098,7 +24098,7 @@ 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: drivers/virt/coco/tsm*.c
>> F: include/linux/tsm.h
>> TRUSTED SERVICES TEE DRIVER
>> diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
>> index ff869d883d95..6f3c0831680b 100644
>> --- a/drivers/virt/coco/Kconfig
>> +++ b/drivers/virt/coco/Kconfig
>> @@ -5,7 +5,8 @@
>> config TSM_REPORTS
>> select CONFIGFS_FS
>> - tristate
>> + select CRYPTO_HASH_INFO
>> + tristate "Trusted Security Module (TSM) sysfs/configfs support"
>
> IMO, sysfs/configfs part is not required in the title.
>
Ok. I'll take it out.
>> source "drivers/virt/coco/efi_secret/Kconfig"
>> diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
>> index c3d07cfc087e..4b108d8df1bd 100644
>> --- a/drivers/virt/coco/Makefile
>> +++ b/drivers/virt/coco/Makefile
>> @@ -2,6 +2,8 @@
>> #
>> # Confidential computing related collateral
>> #
>> +tsm-y += tsm-core.o tsm-mr.o
>> +
>> obj-$(CONFIG_TSM_REPORTS) += tsm.o
>> obj-$(CONFIG_EFI_SECRET) += efi_secret/
>> obj-$(CONFIG_ARM_PKVM_GUEST) += pkvm-guest/
>> diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm-core.c
>> similarity index 99%
>> rename from drivers/virt/coco/tsm.c
>> rename to drivers/virt/coco/tsm-core.c
>> index 9432d4e303f1..ab5269db9c13 100644
>> --- a/drivers/virt/coco/tsm.c
>> +++ b/drivers/virt/coco/tsm-core.c
>> @@ -476,6 +476,9 @@ int tsm_unregister(const struct tsm_ops *ops)
>> }
>> EXPORT_SYMBOL_GPL(tsm_unregister);
>> +int tsm_mr_init(void);
>> +void tsm_mr_exit(void);
>> +
>> static struct config_group *tsm_report_group;
>> static int __init tsm_init(void)
>> @@ -497,12 +500,13 @@ static int __init tsm_init(void)
>> }
>> tsm_report_group = tsm;
>> - return 0;
>> + return tsm_mr_init();
>> }
>> module_init(tsm_init);
>> static void __exit tsm_exit(void)
>> {
>> + tsm_mr_exit();
>> configfs_unregister_default_group(tsm_report_group);
>> configfs_unregister_subsystem(&tsm_configfs);
>> }
>> diff --git a/drivers/virt/coco/tsm-mr.c b/drivers/virt/coco/tsm-mr.c
>> new file mode 100644
>> index 000000000000..8d26e952da6b
>> --- /dev/null
>> +++ b/drivers/virt/coco/tsm-mr.c
>> @@ -0,0 +1,375 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/* Copyright(c) 2024-2025 Intel Corporation. All rights reserved. */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <crypto/hash.h>
>> +#include <crypto/hash_info.h>
>> +#include <linux/kobject.h>
>> +#include <linux/module.h>
>> +#include <linux/tsm.h>
>> +
>> +int tsm_mr_init(void);
>> +void tsm_mr_exit(void);
>> +
>> +enum tmr_dir_battr_index {
>> + TMR_DIR_BA_DIGEST,
>> + TMR_DIR_BA__COUNT,
>
> Why not use single underscore uniformly?
>
DIGEST is a bin attribute while _COUNT is the _count_ of bin attributes.
If w/o the underscore, COUNT would look like another bin attribute.
>> +
>> + TMR_DIR__ALGO_MAX = 4,
>
> Since this is not related to attribute index, why not use #define?
>
Now I'm thinking of making it a CONFIG option, default to 4.
>> +};
>> +
>> +struct tmr_dir {
>> + struct kobject kobj;
>> + struct bin_attribute battrs[TMR_DIR__ALGO_MAX][TMR_DIR_BA__COUNT];
>> + int algo;
>> +};
>> +
>> +struct tmr_provider {
>> + struct kset kset;
>> + struct rw_semaphore rwsem;
>> + struct bin_attribute *mrfiles;
>> + struct tsm_measurement *tmr;
>> + bool in_sync;
>> +};
>> +
>> +static inline struct tmr_provider *tmr_mr_to_provider(const struct
>> tsm_measurement_register *mr,
>> + struct kobject *kobj)
>> +{
>> + if (mr->mr_flags & TSM_MR_F_F)
>> + return container_of(kobj, struct tmr_provider, kset.kobj);
>> + else
>> + return container_of(kobj->kset, struct tmr_provider, kset);
>> +}
>> +
>> +static inline int tmr_call_refresh(struct tmr_provider *pvd,
>> + const struct tsm_measurement_register *mr)
>> +{
>> + int rc;
>> +
>> + rc = pvd->tmr->refresh(pvd->tmr, mr);
>> + if (rc)
>> + pr_warn("%s.refresh(%s) failed %d\n", kobject_name(&pvd-
>> >kset.kobj), mr->mr_name,
>> + rc);
>> + return rc;
>> +}
>> +
>> +static inline int tmr_call_extend(struct tmr_provider *pvd,
>> + const struct tsm_measurement_register *mr, const u8
>> *data)
>> +{
>> + int rc;
>> +
>> + rc = pvd->tmr->extend(pvd->tmr, mr, data);
>> + if (rc)
>> + pr_warn("%s.extend(%s) failed %d\n", kobject_name(&pvd-
>> >kset.kobj), mr->mr_name,
>> + rc);
>> + return rc;
>> +}
>> +
>> +static ssize_t tmr_digest_read(struct file *filp, struct kobject
>> *kobj, struct bin_attribute *attr,
>> + char *page, loff_t off, size_t count)
>> +{
>> + const struct tsm_measurement_register *mr;
>> + struct tmr_provider *pvd;
>> + int rc;
>> +
>> + if (off < 0 || off > attr->size)
>> + return -EINVAL;
>> +
>> + count = min(count, attr->size - (size_t)off);
>> + if (!count)
>> + return count;
>> +
>> + mr = (typeof(mr))attr->private;
>
> I think you don't need to cast it.
>
Thanks! I'll remove it.
>> + pvd = tmr_mr_to_provider(mr, kobj);
>> + rc = down_read_interruptible(&pvd->rwsem);
>> + if (rc)
>> + return rc;
>> +
>> + if ((mr->mr_flags & TSM_MR_F_L) && !pvd->in_sync) {
>> + up_read(&pvd->rwsem);
>> +
>> + rc = down_write_killable(&pvd->rwsem);
>> + if (rc)
>> + return rc;
>> +
>> + if (!pvd->in_sync) {
>
> Since this path is only taken if in_sync is false, do you need to check
> again?
>
Yes, because in_sync could be set to true between up_read and
down_write_killable above.
>> + rc = tmr_call_refresh(pvd, mr);
>> + pvd->in_sync = !rc;
>> + }
>> +
>> + downgrade_write(&pvd->rwsem);
>> + }
>> +
>> + if (!rc)
>> + memcpy(page, mr->mr_value + off, count);
>> +
>> + up_read(&pvd->rwsem);
>> + return rc ?: count;
>> +}
>> +
>> +static ssize_t tmr_digest_write(struct file *filp, struct kobject
>> *kobj, struct bin_attribute *attr,
>> + char *page, loff_t off, size_t count)
>> +{
>> + const struct tsm_measurement_register *mr;
>> + struct tmr_provider *pvd;
>> + ssize_t rc;
>> +
>> + if (off != 0 || count != attr->size)
>> + return -EINVAL;
>> +
>> + mr = (typeof(mr))attr->private;
>> + pvd = tmr_mr_to_provider(mr, kobj);
>> + rc = down_write_killable(&pvd->rwsem);
>> + if (rc)
>> + return rc;
>> +
>> + if (mr->mr_flags & TSM_MR_F_X)
>> + rc = tmr_call_extend(pvd, mr, page);
>> + else
>> + memcpy(mr->mr_value, page, count);
>> +
>> + if (!rc)
>> + pvd->in_sync = false;
>> +
>> + up_write(&pvd->rwsem);
>> + return rc ?: count;
>> +}
>> +
>> +static void tmr_dir_release(struct kobject *kobj)
>> +{
>> + struct tmr_dir *mrd;
>> +
>> + mrd = container_of(kobj, typeof(*mrd), kobj);
>> + kfree(mrd);
>> +}
>> +
>> +static const struct kobj_type tmr_dir_ktype = {
>> + .release = tmr_dir_release,
>> + .sysfs_ops = &kobj_sysfs_ops,
>> +};
>> +
>> +static struct tmr_dir *tmr_dir_create(const struct
>> tsm_measurement_register *mr,
>> + struct tmr_provider *pvd)
>> +{
>> + struct kobject *kobj;
>> + struct tmr_dir *mrd;
>> +
>> + kobj = kset_find_obj(&pvd->kset, mr->mr_name);
>> + if (kobj) {
>> + mrd = container_of(kobj, typeof(*mrd), kobj);
>> + kobject_put(kobj);
>> + if (++mrd->algo >= TMR_DIR__ALGO_MAX) {
>> + --mrd->algo;
>> + return ERR_PTR(-ENOSPC);
>> + }
>> + } else {
>> + int rc;
>> +
>> + mrd = kzalloc(sizeof(*mrd), GFP_KERNEL);
>> + if (!mrd)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + mrd->kobj.kset = &pvd->kset;
>> + rc = kobject_init_and_add(&mrd->kobj, &tmr_dir_ktype, NULL,
>> "%s", mr->mr_name);
>> + if (rc) {
>> + kfree(mrd);
>> + return ERR_PTR(rc);
>> + }
>> + }
>> +
>> + sysfs_bin_attr_init(&mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST]);
>> + mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST].attr.name = "digest";
>
> Since this attribute reflects register value, personally I think "value"
> is more clear
> than "digest". But it is fine either way.
>
This attribute shows up only when its parent dir is a hash algo name
(e.g., "sha384"). So "digest" I believe is more appropriate to refer to
the result of the hash.
>> + if (mr->mr_flags & TSM_MR_F_W)
>> + mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST].attr.mode |=
>> S_IWUSR | S_IWGRP;
>> + if (mr->mr_flags & TSM_MR_F_R)
>> + mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST].attr.mode |= S_IRUGO;
>> +
>> + mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST].size = mr->mr_size;
>> + mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST].read = tmr_digest_read;
>> + mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST].write = tmr_digest_write;
>> + mrd->battrs[mrd->algo][TMR_DIR_BA_DIGEST].private = (void *)mr;
>> +
>> + return mrd;
>> +}
>> +
>> +static void tmr_provider_release(struct kobject *kobj)
>> +{
>> + struct tmr_provider *pvd;
>> +
>> + pvd = container_of(kobj, typeof(*pvd), kset.kobj);
>> + if (!WARN_ON(!list_empty(&pvd->kset.list))) {
>> + kfree(pvd->mrfiles);
>> + kfree(pvd);
>> + }
>> +}
>> +
>> +static const struct kobj_type _mr_provider_ktype = {
>> + .release = tmr_provider_release,
>> + .sysfs_ops = &kobj_sysfs_ops,
>> +};
>> +
>> +static struct kset *tmr_sysfs_root;
>> +
>> +static struct tmr_provider *tmr_provider_create(struct
>> tsm_measurement *tmr)
>> +{
>> + struct tmr_provider *pvd __free(kfree);
>> + int rc;
>> +
>> + pvd = kzalloc(sizeof(*pvd), GFP_KERNEL);
>> + if (!pvd)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + if (!tmr->name || !tmr->mrs || !tmr->refresh || !tmr->extend)
>> + return ERR_PTR(-EINVAL);
>
> Why not add this condition at the top before allocation?
>
Because a few bytes can be saved this way (by not initializing pvd). The
difference (in performance) will only be on the error path, which we
don't care.
>> +
>> + rc = kobject_set_name(&pvd->kset.kobj, "%s", tmr->name);
>> + if (rc)
>> + return ERR_PTR(rc);
>> +
>> + pvd->kset.kobj.kset = tmr_sysfs_root;
>> + pvd->kset.kobj.ktype = &_mr_provider_ktype;
>> + pvd->tmr = tmr;
>> +
>> + init_rwsem(&pvd->rwsem);
>> +
>> + rc = kset_register(&pvd->kset);
>> + if (rc)
>> + return ERR_PTR(rc);
>> +
>> + return_ptr(pvd);
>> +}
>> +
>> +DEFINE_FREE(_unregister_measurement, struct tmr_provider *,
>> + if (!IS_ERR_OR_NULL(_T)) tsm_unregister_measurement(_T->tmr));
>> +
>> +int tsm_register_measurement(struct tsm_measurement *tmr)
>> +{
>> + struct tmr_provider *pvd __free(_unregister_measurement);
>> + int rc, nr;
>> +
>> + pvd = tmr_provider_create(tmr);
>> + if (IS_ERR(pvd))
>> + return PTR_ERR(pvd);
>> +
>> + nr = 0;
>> + for (int i = 0; tmr->mrs[i].mr_name; ++i) {
>> + // flat files are counted and skipped
>> + if (tmr->mrs[i].mr_flags & TSM_MR_F_F) {
>> + ++nr;
>> + continue;
>> + }
>> +
>> + struct tmr_dir *mrd;
>> + struct bin_attribute *battrs[TMR_DIR_BA__COUNT + 1] = {};
>> + struct attribute_group agrp = {
>> + .name = hash_algo_name[tmr->mrs[i].mr_hash],
>> + .bin_attrs = battrs,
>> + };
>> +
>> + mrd = tmr_dir_create(&tmr->mrs[i], pvd);
>> + if (IS_ERR(mrd))
>> + return PTR_ERR(mrd);
>> +
>> + for (int j = 0; j < TMR_DIR_BA__COUNT; ++j)
>> + battrs[j] = &mrd->battrs[mrd->algo][j];
>> +
>> + rc = sysfs_create_group(&mrd->kobj, &agrp);
>> + if (rc)
>> + return rc;
>> + }
>> +
>> + if (nr > 0) {
>> + struct bin_attribute *mrfiles __free(kfree);
>> + struct bin_attribute **battrs __free(kfree);
>> +
>> + mrfiles = kcalloc(nr, sizeof(*mrfiles), GFP_KERNEL);
>> + battrs = kcalloc(nr + 1, sizeof(*battrs), GFP_KERNEL);
>> + if (!battrs || !mrfiles)
>> + return -ENOMEM;
>> +
>> + for (int i = 0, j = 0; tmr->mrs[i].mr_name; ++i) {
>> + if (!(tmr->mrs[i].mr_flags & TSM_MR_F_F))
>> + continue;
>> +
>> + mrfiles[j].attr.name = tmr->mrs[i].mr_name;
>> + mrfiles[j].read = tmr_digest_read;
>> + mrfiles[j].write = tmr_digest_write;
>> + mrfiles[j].size = tmr->mrs[i].mr_size;
>> + mrfiles[j].private = (void *)&tmr->mrs[i];
>> + if (tmr->mrs[i].mr_flags & TSM_MR_F_R)
>> + mrfiles[j].attr.mode |= S_IRUGO;
>> + if (tmr->mrs[i].mr_flags & TSM_MR_F_W)
>> + mrfiles[j].attr.mode |= S_IWUSR | S_IWGRP;
>> +
>> + battrs[j] = &mrfiles[j];
>> + ++j;
>> + }
>> +
>> + struct attribute_group agrp = {
>> + .bin_attrs = battrs,
>> + };
>> + rc = sysfs_create_group(&pvd->kset.kobj, &agrp);
>> + if (rc)
>> + return rc;
>> +
>> + pvd->mrfiles = no_free_ptr(mrfiles);
>> + }
>> +
>> + // initial refresh of MRs
>> + rc = tmr_call_refresh(pvd, NULL);
>> + pvd->in_sync = !rc;
>> +
>> + pvd = NULL; // to avoid being freed automatically
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(tsm_register_measurement);
>> +
>> +static void tmr_put_children(struct kset *kset)
>> +{
>> + struct kobject *p, *n;
>> +
>> + spin_lock(&kset->list_lock);
>> + list_for_each_entry_safe(p, n, &kset->list, entry) {
>> + spin_unlock(&kset->list_lock);
>> + kobject_put(p);
>> + spin_lock(&kset->list_lock);
>> + }
>> + spin_unlock(&kset->list_lock);
>> +}
>> +
>> +int tsm_unregister_measurement(struct tsm_measurement *tmr)
>> +{
>> + struct kobject *kobj;
>> + struct tmr_provider *pvd;
>> +
>> + kobj = kset_find_obj(tmr_sysfs_root, tmr->name);
>> + if (!kobj)
>> + return -ENOENT;
>> +
>> + pvd = container_of(kobj, typeof(*pvd), kset.kobj);
>> + if (pvd->tmr != tmr)
>> + return -EINVAL;
>> +
>> + tmr_put_children(&pvd->kset);
>> + kset_unregister(&pvd->kset);
>> + kobject_put(kobj);
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(tsm_unregister_measurement);
>> +
>> +int tsm_mr_init(void)
>> +{
>> + tmr_sysfs_root = kset_create_and_add("tsm", NULL, kernel_kobj);
>> + if (!tmr_sysfs_root)
>> + return -ENOMEM;
>> + return 0;
>> +}
>> +
>> +void tsm_mr_exit(void)
>> +{
>> + kset_unregister(tmr_sysfs_root);
>> +}
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Provide Trusted Security Module measurements via
>> sysfs");
>> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
>> index 11b0c525be30..624a7b62b85d 100644
>> --- a/include/linux/tsm.h
>> +++ b/include/linux/tsm.h
>> @@ -5,6 +5,7 @@
>> #include <linux/sizes.h>
>> #include <linux/types.h>
>> #include <linux/uuid.h>
>> +#include <uapi/linux/hash_info.h>
>> #define TSM_INBLOB_MAX 64
>> #define TSM_OUTBLOB_MAX SZ_32K
>> @@ -109,4 +110,67 @@ struct tsm_ops {
>> int tsm_register(const struct tsm_ops *ops, void *priv);
>> int tsm_unregister(const struct tsm_ops *ops);
>> +
>> +/**
>> + * struct tsm_measurement_register - describes an architectural
>> measurement register (MR)
>> + * @mr_name: name of the MR
>> + * @mr_value: buffer containing the current value of the MR
>> + * @mr_size: size of the MR - typically the digest size of @mr_hash
>> + * @mr_flags: bitwise OR of flags defined in enum
>> tsm_measurement_register_flag
>> + * @mr_hash: optional hash identifier defined in include/uapi/linux/
>> hash_info.h
>> + *
>> + * A CC guest driver provides this structure to detail the
>> measurement facility supported by the
>> + * underlying CC hardware. After registration via
>> `tsm_register_measurement`, the CC guest driver
>> + * must retain this structure until it is unregistered using
>> `tsm_unregister_measurement`.
>> + */
>> +struct tsm_measurement_register {
>> + const char *mr_name;
>> + void *mr_value;
>> + u32 mr_size;
>> + u32 mr_flags;
>> + enum hash_algo mr_hash;
>> +};
>> +
>> +/**
>> + * enum tsm_measurement_register_flag - properties of an MR
>> + * @TSM_MR_F_X: this MR supports the extension semantics on write
>
> Why not use _E? Before reading the help text, I thought _X is for execute.
>
I was thinking of HTTP and X.509, all extensions are marked by "x".
Anyone else having a preference on _E vs. _X?
>> + * @TSM_MR_F_W: this MR is writable
>> + * @TSM_MR_F_R: this MR is readable. This should typically be set
>> + * @TSM_MR_F_L: this MR is live - writes to other MRs may change this MR
>> + * @TSM_MR_F_F: present this MR as a file (instead of a directory)
>> + * @TSM_MR_F_LIVE: shorthand for L (live) and R (readable)
>> + * @TSM_MR_F_RTMR: shorthand for LIVE and X (extensible)
>> + */
>> +enum tsm_measurement_register_flag {
>> + TSM_MR_F_X = 1,
>> + TSM_MR_F_W = 2,
>
> It is not clear why you want to differentiate between write and extension.
> Please add some help text related to it.
>
R/W is for controlling the file permission of the MR, while X is the
semantics of the MR. I'll try to clarify.
>> + TSM_MR_F_R = 4,
>> + TSM_MR_F_L = 8,
>> + TSM_MR_F_F = 16,
>> + TSM_MR_F_LIVE = TSM_MR_F_L | TSM_MR_F_R,
>> + TSM_MR_F_RTMR = TSM_MR_F_LIVE | TSM_MR_F_X,
>> +};
>> +
>> +#define TSM_MR_(mr,
>> hash) \
>> + .mr_name = #mr, .mr_size = hash##_DIGEST_SIZE, .mr_hash =
>> HASH_ALGO_##hash, \
>> + .mr_flags = TSM_MR_F_R
>> +
>> +/**
>> + * struct tsm_measurement - define CC specific MRs and methods for
>> updating them
>> + * @name: name of the measurement provider
>> + * @mrs: array of MR definitions ending with mr_name set to %NULL
>> + * @refresh: invoked to update the specified MR
>> + * @extend: invoked to extend the specified MR with mr_size bytes
>> + */
>> +struct tsm_measurement {
>> + const char *name;
>> + const struct tsm_measurement_register *mrs;
>> + int (*refresh)(struct tsm_measurement *tmr, const struct
>> tsm_measurement_register *mr);
>> + int (*extend)(struct tsm_measurement *tmr, const struct
>> tsm_measurement_register *mr,
>> + const u8 *data);
>> +};
>> +
>> +int tsm_register_measurement(struct tsm_measurement *tmr);
>> +int tsm_unregister_measurement(struct tsm_measurement *tmr);
>> +
>> #endif /* __TSM_H */
>>
Hi Cedric,
[...]
> +static ssize_t tmr_digest_read(struct file *filp, struct kobject *kobj, struct bin_attribute *attr,
> + char *page, loff_t off, size_t count)
Better to rename 'page' to 'buffer'?
Since page normally implies 4KB alignment but I don't see we need the
alignment here.
> +{
> + const struct tsm_measurement_register *mr;
> + struct tmr_provider *pvd;
> + int rc;
> +
> + if (off < 0 || off > attr->size)
> + return -EINVAL;
> +
> + count = min(count, attr->size - (size_t)off);
> + if (!count)
> + return count;
> +
> + mr = (typeof(mr))attr->private;
> + pvd = tmr_mr_to_provider(mr, kobj);
> + rc = down_read_interruptible(&pvd->rwsem);
> + if (rc)
> + return rc;
> +
> + if ((mr->mr_flags & TSM_MR_F_L) && !pvd->in_sync) {
> + up_read(&pvd->rwsem);
> +
> + rc = down_write_killable(&pvd->rwsem);
> + if (rc)
> + return rc;
> +
> + if (!pvd->in_sync) {
> + rc = tmr_call_refresh(pvd, mr);
> + pvd->in_sync = !rc;
> + }
> +
> + downgrade_write(&pvd->rwsem);
> + }
> +
> + if (!rc)
> + memcpy(page, mr->mr_value + off, count);
> +
> + up_read(&pvd->rwsem);
> + return rc ?: count;
> +}
> +
> +static ssize_t tmr_digest_write(struct file *filp, struct kobject *kobj, struct bin_attribute *attr,
> + char *page, loff_t off, size_t count)
> +{
> + const struct tsm_measurement_register *mr;
> + struct tmr_provider *pvd;
> + ssize_t rc;
> +
> + if (off != 0 || count != attr->size)
> + return -EINVAL;
> +
> + mr = (typeof(mr))attr->private;
> + pvd = tmr_mr_to_provider(mr, kobj);
> + rc = down_write_killable(&pvd->rwsem);
> + if (rc)
> + return rc;
> +
> + if (mr->mr_flags & TSM_MR_F_X)
> + rc = tmr_call_extend(pvd, mr, page);
> + else
> + memcpy(mr->mr_value, page, count);
> +
> + if (!rc)
> + pvd->in_sync = false;
> +
> + up_write(&pvd->rwsem);
> + return rc ?: count;
> +}
The logic around using pvd->in_sync is kinda complicated. MR operations
seem like a classic reader/writer contention problem and I am not sure
why pvd->in_sync is needed. Could you help to clarify?
[...]
> +
> +/**
> + * struct tsm_measurement_register - describes an architectural measurement register (MR)
> + * @mr_name: name of the MR
> + * @mr_value: buffer containing the current value of the MR
> + * @mr_size: size of the MR - typically the digest size of @mr_hash
> + * @mr_flags: bitwise OR of flags defined in enum tsm_measurement_register_flag
> + * @mr_hash: optional hash identifier defined in include/uapi/linux/hash_info.h
> + *
> + * A CC guest driver provides this structure to detail the measurement facility supported by the
> + * underlying CC hardware. After registration via `tsm_register_measurement`, the CC guest driver
> + * must retain this structure until it is unregistered using `tsm_unregister_measurement`.
> + */
> +struct tsm_measurement_register {
> + const char *mr_name;
> + void *mr_value;
> + u32 mr_size;
> + u32 mr_flags;
> + enum hash_algo mr_hash;
> +};
> +
> +/**
> + * enum tsm_measurement_register_flag - properties of an MR
> + * @TSM_MR_F_X: this MR supports the extension semantics on write
> + * @TSM_MR_F_W: this MR is writable
Why a MR can be written w/o being extended? What is the use case of this?
> + * @TSM_MR_F_R: this MR is readable. This should typically be set
> + * @TSM_MR_F_L: this MR is live - writes to other MRs may change this MR
Why one MR can be changed by writing to other MRs?
> + * @TSM_MR_F_F: present this MR as a file (instead of a directory)
> + * @TSM_MR_F_LIVE: shorthand for L (live) and R (readable)
> + * @TSM_MR_F_RTMR: shorthand for LIVE and X (extensible)
> + */
> +enum tsm_measurement_register_flag {
> + TSM_MR_F_X = 1,
> + TSM_MR_F_W = 2,
> + TSM_MR_F_R = 4,
> + TSM_MR_F_L = 8,
> + TSM_MR_F_F = 16,
> + TSM_MR_F_LIVE = TSM_MR_F_L | TSM_MR_F_R,
> + TSM_MR_F_RTMR = TSM_MR_F_LIVE | TSM_MR_F_X,
> +};
I am not sure whether we need so many flags. To me seems like we only need:
- TSM_MR_ENABLED: The MR has been initialized with a certain algo.
- TSM_MR_UNLOCKED: The MR is writable and any write will extend it.
- TSM_MR_LOCKED: The MR is locked and finalized.
The TSM_MR_ENABLED may not be needed either, but I think it's better to
have it so that the kernel can reject both read/write from userspace.
> +
> +#define TSM_MR_(mr, hash) \
> + .mr_name = #mr, .mr_size = hash##_DIGEST_SIZE, .mr_hash = HASH_ALGO_##hash, \
> + .mr_flags = TSM_MR_F_R
> +
> +/**
> + * struct tsm_measurement - define CC specific MRs and methods for updating them
> + * @name: name of the measurement provider
> + * @mrs: array of MR definitions ending with mr_name set to %NULL
> + * @refresh: invoked to update the specified MR
> + * @extend: invoked to extend the specified MR with mr_size bytes
> + */
> +struct tsm_measurement {
> + const char *name;
> + const struct tsm_measurement_register *mrs;
> + int (*refresh)(struct tsm_measurement *tmr, const struct tsm_measurement_register *mr);
> + int (*extend)(struct tsm_measurement *tmr, const struct tsm_measurement_register *mr,
> + const u8 *data);
> +};
From the description above, I don't quite follow what does ->refresh()
do exactly. Could you clarify why we need it?
Hi Kai,
On 2/16/2025 6:17 PM, Huang, Kai wrote:
> Hi Cedric,
>
> [...]
>
>> +static ssize_t tmr_digest_read(struct file *filp, struct kobject
>> *kobj, struct bin_attribute *attr,
>> + char *page, loff_t off, size_t count)
>
> Better to rename 'page' to 'buffer'?
>
> Since page normally implies 4KB alignment but I don't see we need the
> alignment here.
>
'page' was used here to imply the size of the buffer cannot exceed a
page (which is the current behavior of the kernel). But I agree with you
and will make the changes.
[...]
> The logic around using pvd->in_sync is kinda complicated. MR operations
> seem like a classic reader/writer contention problem and I am not sure
> why pvd->in_sync is needed. Could you help to clarify?
>
If in_sync is true, then "refresh()" will NOT be invoked on reads from
"live" MRs.
For example, on TDX, if an RTMR has NOT been extended since the last
read, then the next read will return the cached copy of the RTMR value -
i.e., saving a "refresh()" call (which must issue TDCALL[TDG.MR.REPORT]
to reread all MRs and can be slow).
> [...]
>
>> +
>> +/**
>> + * struct tsm_measurement_register - describes an architectural
>> measurement register (MR)
>> + * @mr_name: name of the MR
>> + * @mr_value: buffer containing the current value of the MR
>> + * @mr_size: size of the MR - typically the digest size of @mr_hash
>> + * @mr_flags: bitwise OR of flags defined in enum
>> tsm_measurement_register_flag
>> + * @mr_hash: optional hash identifier defined in include/uapi/linux/
>> hash_info.h
>> + *
>> + * A CC guest driver provides this structure to detail the
>> measurement facility supported by the
>> + * underlying CC hardware. After registration via
>> `tsm_register_measurement`, the CC guest driver
>> + * must retain this structure until it is unregistered using
>> `tsm_unregister_measurement`.
>> + */
>> +struct tsm_measurement_register {
>> + const char *mr_name;
>> + void *mr_value;
>> + u32 mr_size;
>> + u32 mr_flags;
>> + enum hash_algo mr_hash;
>> +};
>> +
>> +/**
>> + * enum tsm_measurement_register_flag - properties of an MR
>> + * @TSM_MR_F_X: this MR supports the extension semantics on write
>> + * @TSM_MR_F_W: this MR is writable
>
> Why a MR can be written w/o being extended? What is the use case of this?
>
This is because "write" may not be the only way to extend an RTMR. For
example, the current ABI proposed by this patch can be considered "MR
centric", meaning it's the application that takes care of what to hash,
using what algorithm, and which RTMR to extend. However, theoretically,
applications should only be concerned the integrity of some sequence of
events (the event log). Therefore, there could be a "log centric" ABI
that allows applications to integrity-protect its logs in a CC-arch
agnostic manner. And if that's the case, RTMRs may be marked RO ("X w/o
W") to prevent direct extension.
The use of "W w/o X" is to support pseudo-MRs. For example, `reportdata`
is such a pseudo-MR that is W but not X. So an application can request a
TDREPORT by a write to `reportdata` followed by a read from `report0`.
>> + * @TSM_MR_F_R: this MR is readable. This should typically be set
>> + * @TSM_MR_F_L: this MR is live - writes to other MRs may change this MR
>
> Why one MR can be changed by writing to other MRs?
>
Good catch! I'll fix the comment.
>> + * @TSM_MR_F_F: present this MR as a file (instead of a directory)
>> + * @TSM_MR_F_LIVE: shorthand for L (live) and R (readable)
>> + * @TSM_MR_F_RTMR: shorthand for LIVE and X (extensible)
>> + */
>> +enum tsm_measurement_register_flag {
>> + TSM_MR_F_X = 1,
>> + TSM_MR_F_W = 2,
>> + TSM_MR_F_R = 4,
>> + TSM_MR_F_L = 8,
>> + TSM_MR_F_F = 16,
>> + TSM_MR_F_LIVE = TSM_MR_F_L | TSM_MR_F_R,
>> + TSM_MR_F_RTMR = TSM_MR_F_LIVE | TSM_MR_F_X,
>> +};
>
> I am not sure whether we need so many flags. To me seems like we only
> need:
>
> - TSM_MR_ENABLED: The MR has been initialized with a certain algo.
> - TSM_MR_UNLOCKED: The MR is writable and any write will extend it.
> - TSM_MR_LOCKED: The MR is locked and finalized.
>
W/X are independent and both necessary (see my previous explanation on
"X w/o W" and "W w/o X").
I'm not sure if there are non-readable MRs. But theoretically,
applications inside a TVM (CC guest) may not need to read any MR values.
Therefore, there could be CC archs (in future) that do not support
reading all MRs within a guest. And because of that, I decided to keep R
as an independent bit.
L is to indicate an MR's value may not match its last write.
F is for CC guest to expose (pseudo) MRs that may not have an associated
hash algorithm (e.g., `report0` on TDX).
LOCKED/UNLOCKED, from attestation perspective, is NOT a functional but a
verifiable security property, which is usually implemented by extending
a special token to the RTMR.
> The TSM_MR_ENABLED may not be needed either, but I think it's better to
> have it so that the kernel can reject both read/write from userspace.
>
I'm not sure what a "disabled" MR is and its implication from
attestation perspective.
>> +
>> +#define TSM_MR_(mr,
>> hash) \
>> + .mr_name = #mr, .mr_size = hash##_DIGEST_SIZE, .mr_hash =
>> HASH_ALGO_##hash, \
>> + .mr_flags = TSM_MR_F_R
>> +
>> +/**
>> + * struct tsm_measurement - define CC specific MRs and methods for
>> updating them
>> + * @name: name of the measurement provider
>> + * @mrs: array of MR definitions ending with mr_name set to %NULL
>> + * @refresh: invoked to update the specified MR
>> + * @extend: invoked to extend the specified MR with mr_size bytes
>> + */
>> +struct tsm_measurement {
>> + const char *name;
>> + const struct tsm_measurement_register *mrs;
>> + int (*refresh)(struct tsm_measurement *tmr, const struct
>> tsm_measurement_register *mr);
>> + int (*extend)(struct tsm_measurement *tmr, const struct
>> tsm_measurement_register *mr,
>> + const u8 *data);
>> +};
>
> From the description above, I don't quite follow what does ->refresh()
> do exactly. Could you clarify why we need it?
I'll fix the comment.
Basically, refresh() brings all cached MR values up to date. The
parameter `mr` indicate which MR that has triggered the refresh. On TDX,
the 1st read after a write to any RTMR will trigger refresh() to reread
all MRs by TDG.MR.REPORT, while subsequent reads will simply return the
cached values until the next write to any RTMRs.
-Cedric
> > > +/**
> > > + * enum tsm_measurement_register_flag - properties of an MR
> > > + * @TSM_MR_F_X: this MR supports the extension semantics on write
> > > + * @TSM_MR_F_W: this MR is writable
> >
> > Why a MR can be written w/o being extended? What is the use case of this?
> >
>
> This is because "write" may not be the only way to extend an RTMR. For
> example, the current ABI proposed by this patch can be considered "MR
> centric", meaning it's the application that takes care of what to hash,
> using what algorithm, and which RTMR to extend.
>
[...]
> However, theoretically,
> applications should only be concerned the integrity of some sequence of
> events (the event log). Therefore, there could be a "log centric" ABI
> that allows applications to integrity-protect its logs in a CC-arch
> agnostic manner.
>
I agree "log centric" ABI could be useful. I don't know a lot of the format of
"event log", but I am thinking that making sure "integrity of some sequence of
events" may not be good enough -- we actually need to make sure "integrity of
each component" that get involved in those events.
E.g., a guest wants to load some particular kernel module during boot and wants
to make sure the correct one gets loaded. Userspace can trigger an event of
"loading that module" and get this *event log* verified. But w/o getting the
kernel module binary itself measured as part of this step, we cannot really be
sure whether this step is compromised or not. In this case, the userspace may
still need to (write and) extend the MR(s).
> And if that's the case, RTMRs may be marked RO ("X w/o
> W") to prevent direct extension.
Sorry I don't quite follow why RO is enough for "log centric" ABI. Could you
elaborate a bit?
>
> The use of "W w/o X" is to support pseudo-MRs. For example, `reportdata`
> is such a pseudo-MR that is W but not X. So an application can request a
> TDREPORT by a write to `reportdata` followed by a read from `report0`.
I am a little bit confused. This series is about exposing "measurement
registers" to userspace, so I thought there should be at least some
"measurement" get involved for any entry that is report to userspace.
'reportdata' is more like the nonce embedded to the attestation report, and it
doesn't involve any measurement.
I can see why you want to expose 'reportdata' to userspace, but calling
'reportdata' as measurement register seems unfit.
>
> > > + * @TSM_MR_F_R: this MR is readable. This should typically be set
> > > + * @TSM_MR_F_L: this MR is live - writes to other MRs may change this MR
> >
> > Why one MR can be changed by writing to other MRs?
> >
>
> Good catch! I'll fix the comment.
>
> > > + * @TSM_MR_F_F: present this MR as a file (instead of a directory)
> > > + * @TSM_MR_F_LIVE: shorthand for L (live) and R (readable)
> > > + * @TSM_MR_F_RTMR: shorthand for LIVE and X (extensible)
> > > + */
> > > +enum tsm_measurement_register_flag {
> > > + TSM_MR_F_X = 1,
> > > + TSM_MR_F_W = 2,
> > > + TSM_MR_F_R = 4,
> > > + TSM_MR_F_L = 8,
> > > + TSM_MR_F_F = 16,
> > > + TSM_MR_F_LIVE = TSM_MR_F_L | TSM_MR_F_R,
> > > + TSM_MR_F_RTMR = TSM_MR_F_LIVE | TSM_MR_F_X,
> > > +};
> >
> > I am not sure whether we need so many flags. To me seems like we only
> > need:
> >
> > - TSM_MR_ENABLED: The MR has been initialized with a certain algo.
> > - TSM_MR_UNLOCKED: The MR is writable and any write will extend it.
> > - TSM_MR_LOCKED: The MR is locked and finalized.
> >
>
> W/X are independent and both necessary (see my previous explanation on
> "X w/o W" and "W w/o X").
>
> I'm not sure if there are non-readable MRs. But theoretically,
> applications inside a TVM (CC guest) may not need to read any MR values.
> Therefore, there could be CC archs (in future) that do not support
> reading all MRs within a guest. And because of that, I decided to keep R
> as an independent bit.
[...]
>
> L is to indicate an MR's value may not match its last write.
"L" doesn't seem to be able to reflect the MR value is out-of-sync. :-)
What does it stand for?
>
> F is for CC guest to expose (pseudo) MRs that may not have an associated
> hash algorithm (e.g., `report0` on TDX).
OK. But my thinking is such MR actually isn't MR at all.
>
> LOCKED/UNLOCKED, from attestation perspective, is NOT a functional but a
> verifiable security property, which is usually implemented by extending
> a special token to the RTMR.
>
> > The TSM_MR_ENABLED may not be needed either, but I think it's better to
> > have it so that the kernel can reject both read/write from userspace.
> >
> I'm not sure what a "disabled" MR is and its implication from
> attestation perspective.
I was thinking from the perspective that userspace may only be interested in one
particular MR. If that MR is not used, I suppose it should have default value
0. But I was thinking that "refusing userspace to read" may be better than
"returning 0 to userspace" for a particular MR, if it is not used.
But from attestation's perspective, I tend to agree with you that "disabled MR"
may not be helpful. We need to send the whole attestation report to the
verifier anyway and the verifier should only care about whether the MR values in
the report matches what the verifier knows.
>
> > > +
> > > +#define TSM_MR_(mr,
> > > hash) \
> > > + .mr_name = #mr, .mr_size = hash##_DIGEST_SIZE, .mr_hash =
> > > HASH_ALGO_##hash, \
> > > + .mr_flags = TSM_MR_F_R
> > > +
> > > +/**
> > > + * struct tsm_measurement - define CC specific MRs and methods for
> > > updating them
> > > + * @name: name of the measurement provider
> > > + * @mrs: array of MR definitions ending with mr_name set to %NULL
> > > + * @refresh: invoked to update the specified MR
> > > + * @extend: invoked to extend the specified MR with mr_size bytes
> > > + */
> > > +struct tsm_measurement {
> > > + const char *name;
> > > + const struct tsm_measurement_register *mrs;
> > > + int (*refresh)(struct tsm_measurement *tmr, const struct
> > > tsm_measurement_register *mr);
> > > + int (*extend)(struct tsm_measurement *tmr, const struct
> > > tsm_measurement_register *mr,
> > > + const u8 *data);
> > > +};
> >
> > From the description above, I don't quite follow what does ->refresh()
> > do exactly. Could you clarify why we need it?
>
> I'll fix the comment.
Thanks.
>
> Basically, refresh() brings all cached MR values up to date. The
> parameter `mr` indicate which MR that has triggered the refresh. On TDX,
> the 1st read after a write to any RTMR will trigger refresh() to reread
> all MRs by TDG.MR.REPORT, while subsequent reads will simply return the
> cached values until the next write to any RTMRs.
Yeah. I also think adding some comments around the code using pvd->in_sync
would be helpful, as I mentioned in another reply.
On 2/18/2025 3:14 AM, Huang, Kai wrote:
>
>>>> +/**
>>>> + * enum tsm_measurement_register_flag - properties of an MR
>>>> + * @TSM_MR_F_X: this MR supports the extension semantics on write
>>>> + * @TSM_MR_F_W: this MR is writable
>>>
>>> Why a MR can be written w/o being extended? What is the use case of this?
>>>
>>
>> This is because "write" may not be the only way to extend an RTMR. For
>> example, the current ABI proposed by this patch can be considered "MR
>> centric", meaning it's the application that takes care of what to hash,
>> using what algorithm, and which RTMR to extend.
>>
>
> [...]
>
>> However, theoretically,
>> applications should only be concerned the integrity of some sequence of
>> events (the event log). Therefore, there could be a "log centric" ABI
>> that allows applications to integrity-protect its logs in a CC-arch
>> agnostic manner.
>>
>
> I agree "log centric" ABI could be useful. I don't know a lot of the format of
> "event log", but I am thinking that making sure "integrity of some sequence of
> events" may not be good enough -- we actually need to make sure "integrity of
> each component" that get involved in those events.
>
By "some sequence of events", I was talking from the perspective of an
application. Different applications will generate independent sequences
of events and because different CC archs offer different types and
numbers of MRs that use different hash algorithms, what we need is a SW
arch that can "pack" those sequences into something that can be
integrity-protected by the HW resource from the underlying CC arch, then
"unpack" it back to independent sequences to be verified/appraised by
potentially independent entities. The "log centric" ABI will be part of
such an architecture. This is too big a topic to solve by this patch.
For now, what matters is just not to create roadblocks.
> E.g., a guest wants to load some particular kernel module during boot and wants
> to make sure the correct one gets loaded. Userspace can trigger an event of
> "loading that module" and get this *event log* verified. But w/o getting the
> kernel module binary itself measured as part of this step, we cannot really be
> sure whether this step is compromised or not. In this case, the userspace may
> still need to (write and) extend the MR(s).
>
By the term "event", it usually implies accompanying "event data", which
could be/contain the measurements of the kernel modules.
You are talking about actual uses of RTMRs and this patch aims to enable
those exact uses. :-)
>> And if that's the case, RTMRs may be marked RO ("X w/o
>> W") to prevent direct extension.
>
> Sorry I don't quite follow why RO is enough for "log centric" ABI. Could you
> elaborate a bit?
>
My apologies for the confusion again! R/W controls the file permissions
of the sysfs attributes, while X is the semantics of the MR. Clearing W
prevents direct "write" to the attribute (by user mode) but doesn't
prevent the MR from being extended through other interfaces. It isn't
obvious in this patch because the "log centric" ABI has been taken out.
It was originally proposed in
https://lore.kernel.org/all/20240907-tsm-rtmr-v1-0-12fc4d43d4e7@intel.com/
>>
>> The use of "W w/o X" is to support pseudo-MRs. For example, `reportdata`
>> is such a pseudo-MR that is W but not X. So an application can request a
>> TDREPORT by a write to `reportdata` followed by a read from `report0`.
>
> I am a little bit confused. This series is about exposing "measurement
> registers" to userspace, so I thought there should be at least some
> "measurement" get involved for any entry that is report to userspace.
>
> 'reportdata' is more like the nonce embedded to the attestation report, and it
> doesn't involve any measurement.
>
> I can see why you want to expose 'reportdata' to userspace, but calling
> 'reportdata' as measurement register seems unfit.
>
Glad that you see why! I called it a pseudo-MR but there could be better
names. The intention of this patch is to create a sysfs tree that offers
all measurement related functionalities to user mode applications.
`reportdata` plays a role here because it usually carries the digest of
something that the guest wants to attest to.
>>
>>>> + * @TSM_MR_F_R: this MR is readable. This should typically be set
>>>> + * @TSM_MR_F_L: this MR is live - writes to other MRs may change this MR
>>>
>>> Why one MR can be changed by writing to other MRs?
>>>
>>
>> Good catch! I'll fix the comment.
>>
>>>> + * @TSM_MR_F_F: present this MR as a file (instead of a directory)
>>>> + * @TSM_MR_F_LIVE: shorthand for L (live) and R (readable)
>>>> + * @TSM_MR_F_RTMR: shorthand for LIVE and X (extensible)
>>>> + */
>>>> +enum tsm_measurement_register_flag {
>>>> + TSM_MR_F_X = 1,
>>>> + TSM_MR_F_W = 2,
>>>> + TSM_MR_F_R = 4,
>>>> + TSM_MR_F_L = 8,
>>>> + TSM_MR_F_F = 16,
>>>> + TSM_MR_F_LIVE = TSM_MR_F_L | TSM_MR_F_R,
>>>> + TSM_MR_F_RTMR = TSM_MR_F_LIVE | TSM_MR_F_X,
>>>> +};
>>>
>>> I am not sure whether we need so many flags. To me seems like we only
>>> need:
>>>
>>> - TSM_MR_ENABLED: The MR has been initialized with a certain algo.
>>> - TSM_MR_UNLOCKED: The MR is writable and any write will extend it.
>>> - TSM_MR_LOCKED: The MR is locked and finalized.
>>>
>>
>> W/X are independent and both necessary (see my previous explanation on
>> "X w/o W" and "W w/o X").
>>
>> I'm not sure if there are non-readable MRs. But theoretically,
>> applications inside a TVM (CC guest) may not need to read any MR values.
>> Therefore, there could be CC archs (in future) that do not support
>> reading all MRs within a guest. And because of that, I decided to keep R
>> as an independent bit.
>
> [...]
>
>>
>> L is to indicate an MR's value may not match its last write.
>
> "L" doesn't seem to be able to reflect the MR value is out-of-sync. :-)
>
> What does it stand for?
>
L stands for Live. It indicates to the TSM MR core that the value of the
MR is different than the last value written, hence should issue
refresh() to read the value back from HW (instead of using the cached
value obtained from the last write).
>>
>> F is for CC guest to expose (pseudo) MRs that may not have an associated
>> hash algorithm (e.g., `report0` on TDX).
>
> OK. But my thinking is such MR actually isn't MR at all.
>
Besides Measurement Register, MR can also stand for MeasuRement. Not to
debate what MR stands for, but this patch aims to capture/expose all
measurement related functionalities. `report0` offers less understood
yet important info beyond measurement registers, such as CPUSVN and
measurements of the TDX module, so is considered within the scope.
>>
>> LOCKED/UNLOCKED, from attestation perspective, is NOT a functional but a
>> verifiable security property, which is usually implemented by extending
>> a special token to the RTMR.
>>
>>> The TSM_MR_ENABLED may not be needed either, but I think it's better to
>>> have it so that the kernel can reject both read/write from userspace.
>>>
>> I'm not sure what a "disabled" MR is and its implication from
>> attestation perspective.
>
> I was thinking from the perspective that userspace may only be interested in one
> particular MR. If that MR is not used, I suppose it should have default value
> 0. But I was thinking that "refusing userspace to read" may be better than
> "returning 0 to userspace" for a particular MR, if it is not used.
>
I'd try not to predict what applications need but focus on what the HW
offers. HW features are usually motivated by specific usages, but their
actual uses are usually way beyond.
Moreover, the traditional Linux file ownership/permissions can satisfy
this need very easily.
> But from attestation's perspective, I tend to agree with you that "disabled MR"
> may not be helpful. We need to send the whole attestation report to the
> verifier anyway and the verifier should only care about whether the MR values in
> the report matches what the verifier knows.
>
>>
>>>> +
>>>> +#define TSM_MR_(mr,
>>>> hash) \
>>>> + .mr_name = #mr, .mr_size = hash##_DIGEST_SIZE, .mr_hash =
>>>> HASH_ALGO_##hash, \
>>>> + .mr_flags = TSM_MR_F_R
>>>> +
>>>> +/**
>>>> + * struct tsm_measurement - define CC specific MRs and methods for
>>>> updating them
>>>> + * @name: name of the measurement provider
>>>> + * @mrs: array of MR definitions ending with mr_name set to %NULL
>>>> + * @refresh: invoked to update the specified MR
>>>> + * @extend: invoked to extend the specified MR with mr_size bytes
>>>> + */
>>>> +struct tsm_measurement {
>>>> + const char *name;
>>>> + const struct tsm_measurement_register *mrs;
>>>> + int (*refresh)(struct tsm_measurement *tmr, const struct
>>>> tsm_measurement_register *mr);
>>>> + int (*extend)(struct tsm_measurement *tmr, const struct
>>>> tsm_measurement_register *mr,
>>>> + const u8 *data);
>>>> +};
>>>
>>> From the description above, I don't quite follow what does ->refresh()
>>> do exactly. Could you clarify why we need it?
>>
>> I'll fix the comment.
>
> Thanks.
>
>>
>> Basically, refresh() brings all cached MR values up to date. The
>> parameter `mr` indicate which MR that has triggered the refresh. On TDX,
>> the 1st read after a write to any RTMR will trigger refresh() to reread
>> all MRs by TDG.MR.REPORT, while subsequent reads will simply return the
>> cached values until the next write to any RTMRs.
>
> Yeah. I also think adding some comments around the code using pvd->in_sync
> would be helpful, as I mentioned in another reply.
>
On Mon, 2025-02-17 at 13:17 +1300, Huang, Kai wrote:
> Hi Cedric,
>
> [...]
>
> > +static ssize_t tmr_digest_read(struct file *filp, struct kobject *kobj, struct bin_attribute *attr,
> > + char *page, loff_t off, size_t count)
>
> Better to rename 'page' to 'buffer'?
>
> Since page normally implies 4KB alignment but I don't see we need the
> alignment here.
>
> > +{
> > + const struct tsm_measurement_register *mr;
> > + struct tmr_provider *pvd;
> > + int rc;
> > +
> > + if (off < 0 || off > attr->size)
> > + return -EINVAL;
> > +
> > + count = min(count, attr->size - (size_t)off);
> > + if (!count)
> > + return count;
> > +
> > + mr = (typeof(mr))attr->private;
> > + pvd = tmr_mr_to_provider(mr, kobj);
> > + rc = down_read_interruptible(&pvd->rwsem);
> > + if (rc)
> > + return rc;
> > +
> > + if ((mr->mr_flags & TSM_MR_F_L) && !pvd->in_sync) {
> > + up_read(&pvd->rwsem);
> > +
> > + rc = down_write_killable(&pvd->rwsem);
> > + if (rc)
> > + return rc;
> > +
> > + if (!pvd->in_sync) {
> > + rc = tmr_call_refresh(pvd, mr);
> > + pvd->in_sync = !rc;
> > + }
> > +
> > + downgrade_write(&pvd->rwsem);
> > + }
> > +
> > + if (!rc)
> > + memcpy(page, mr->mr_value + off, count);
> > +
> > + up_read(&pvd->rwsem);
> > + return rc ?: count;
> > +}
> > +
> > +static ssize_t tmr_digest_write(struct file *filp, struct kobject *kobj, struct bin_attribute *attr,
> > + char *page, loff_t off, size_t count)
> > +{
> > + const struct tsm_measurement_register *mr;
> > + struct tmr_provider *pvd;
> > + ssize_t rc;
> > +
> > + if (off != 0 || count != attr->size)
> > + return -EINVAL;
> > +
> > + mr = (typeof(mr))attr->private;
> > + pvd = tmr_mr_to_provider(mr, kobj);
> > + rc = down_write_killable(&pvd->rwsem);
> > + if (rc)
> > + return rc;
> > +
> > + if (mr->mr_flags & TSM_MR_F_X)
> > + rc = tmr_call_extend(pvd, mr, page);
> > + else
> > + memcpy(mr->mr_value, page, count);
> > +
> > + if (!rc)
> > + pvd->in_sync = false;
> > +
> > + up_write(&pvd->rwsem);
> > + return rc ?: count;
> > +}
>
> The logic around using pvd->in_sync is kinda complicated. MR operations
> seem like a classic reader/writer contention problem and I am not sure
> why pvd->in_sync is needed. Could you help to clarify?
>
> [...]
[...]
> >
> > +#define TSM_MR_(mr, hash) \
> > + .mr_name = #mr, .mr_size = hash##_DIGEST_SIZE, .mr_hash = HASH_ALGO_##hash, \
> > + .mr_flags = TSM_MR_F_R
> > +
> > +/**
> > + * struct tsm_measurement - define CC specific MRs and methods for updating them
> > + * @name: name of the measurement provider
> > + * @mrs: array of MR definitions ending with mr_name set to %NULL
> > + * @refresh: invoked to update the specified MR
> > + * @extend: invoked to extend the specified MR with mr_size bytes
> > + */
> > +struct tsm_measurement {
> > + const char *name;
> > + const struct tsm_measurement_register *mrs;
> > + int (*refresh)(struct tsm_measurement *tmr, const struct tsm_measurement_register *mr);
> > + int (*extend)(struct tsm_measurement *tmr, const struct tsm_measurement_register *mr,
> > + const u8 *data);
> > +};
>
> From the description above, I don't quite follow what does ->refresh()
> do exactly. Could you clarify why we need it?
>
After reading patch 4, I figured out the purpose of the pvd->in_sync and the
refresh() myself:
Each call of ->extend() will update the value of the MR, but only in the
hardware/firmware. The MR value maintained by the kernel is unchanged so it
will be out-of-date after the ->extend(). Therefore the kernel needs to keep
them synced first when userspace reads the MR, using the ->refresh().
It's also feasible to always sync/refresh after each extend(), eliminating the
pvd->in_sync logic completely, but it's not desired -- because the sync
operation talks to the hardware/firmware thus could be time-consuming, and we in
general only needs to sync before reading the value to the userspace. E.g.,
this saves a lot of sync operation if userspace extends the MR multi-times with
large chunk of data and then reads the final value once.
So the pvd->in_sync logic seems useful to me, but I think it would be better to
a comment to explain this in both tmr_digest_read() and tmr_digest_write().
E.g., something like below before the code which checks pvd->in_sync in the
tmr_digest_read()?
/*
* Each write-and-extend to the MR will update the MR, but only in
* TSM hardware/firmware, leaving the MR value maintained by the kernel
* out-of-date. Sync MR value in TSM hardware/firmware to the kernel
* before returning to the userspace in this case.
*/
if ((mr->mr_flags & TSM_MR_F_L) && !pvd->in_sync) {
...
The description of the refresh() callback is also not clear to me. Perhaps
something like below?
/*
* ...
* @refresh: sync MR value in TSM hardware/firmware to the kernel
buffer
* ...
*/
Hi Cedric,
kernel test robot noticed the following build warnings:
[auto build test WARNING on a64dcfb451e254085a7daee5fe51bf22959d52d3]
url: https://github.com/intel-lab-lkp/linux/commits/Cedric-Xing/tsm-Add-TVM-Measurement-Register-support/20250213-102639
base: a64dcfb451e254085a7daee5fe51bf22959d52d3
patch link: https://lore.kernel.org/r/20250212-tdx-rtmr-v1-1-9795dc49e132%40intel.com
patch subject: [PATCH 1/4] tsm: Add TVM Measurement Register support
config: nios2-kismet-CONFIG_CRYPTO_HASH_INFO-CONFIG_TSM_REPORTS-0-0 (https://download.01.org/0day-ci/archive/20250214/202502140854.9CZ1xPmC-lkp@intel.com/config)
reproduce: (https://download.01.org/0day-ci/archive/20250214/202502140854.9CZ1xPmC-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502140854.9CZ1xPmC-lkp@intel.com/
kismet warnings: (new ones prefixed by >>)
>> kismet: WARNING: unmet direct dependencies detected for CRYPTO_HASH_INFO when selected by TSM_REPORTS
WARNING: unmet direct dependencies detected for CRYPTO_HASH_INFO
Depends on [n]: CRYPTO [=n]
Selected by [y]:
- TSM_REPORTS [=y] && VIRT_DRIVERS [=y]
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
© 2016 - 2025 Red Hat, Inc.