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

From: Xing, Cedric
Date: Fri Apr 11 2025 - 00:02:47 EST

Next message: Longfang Liu: "[PATCH v7 5/6] hisi_acc_vfio_pci: bugfix live migration function without VF device driver"
Previous message: Longfang Liu: "[PATCH v7 4/6] hisi_acc_vfio_pci: bugfix the problem of uninstalling driver"
In reply to: Dan Williams: "Re: [PATCH v3 1/5] tsm-mr: Add TVM Measurement Register support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 4/8/2025 7:27 PM, Dan Williams wrote:

Cedric Xing wrote:

[...]

---
MAINTAINERS | 4 +-
drivers/virt/coco/Kconfig | 5 ++
drivers/virt/coco/Makefile | 1 +
drivers/virt/coco/tsm-mr.c | 209 +++++++++++++++++++++++++++++++++++++++++++++
include/linux/tsm-mr.h | 93 ++++++++++++++++++++

I note that the pending proposals for TEE I/O suggests splitting
drivers/virt/coco/ into drivers/virt/coco/{host,guest} [1] [2] [3].

[1]: http://lore.kernel.org/174107246021.1288555.7203769833791489618.stgit@xxxxxxxxxxxxxxxxxxxxxxxxx
[2]: http://lore.kernel.org/20250218111017.491719-8-aik@xxxxxxx
[3]: http://lore.kernel.org/20250218111017.491719-17-aik@xxxxxxx

So if I take this through devsec.git I will get that rename queued and
ask you to rebase on top of that.

No problem.

[...]

+ /*
+ * @ctx->in_sync indicates if any MRs have been written since the last
+ * ctx->refresh() call. When @ctx->in_sync is false, ctx->refresh() is
+ * necessary to sync the cached values of all live MRs (i.e., with
+ * %TSM_MR_F_LIVE set) with the underlying hardware.
+ */

Code comments should add to the understanding of the code, not simply
restate the code in prose. So I would replace this comment with some
non-obvious insight to aid future maintenance, something like:

/*
* Note that the typical read path for MRs is via an attestation report,
* this is why the ->write() path does not automatically ->refresh()
* invalidated data for TSM_MR_LIVE registers. The use case for reading
* back a individual hash-extending-write to an MR is for debug not
* attestation.
*/

...at least an explanation like that would have helped me understand the
locking and caching model of this implementation.

The reasoning behind this involves not only ->refresh() and ->write() but also LIVE and ->in_sync. Generally, both ->refresh() and ->write() could be expensive so we are trying to do only the minimum. I'll add comments to the definition of this context structure.

[...]

+static ssize_t tm_digest_write(struct file *filp, struct kobject *kobj,
+ const struct bin_attribute *attr, char *buffer,
+ loff_t off, size_t count)
+{
+ struct tm_context *ctx;
+ const struct tsm_measurement_register *mr;
+ ssize_t rc;
+
+ /* partial writes are not supported */
+ if (off != 0 || count != attr->size)
+ return -EINVAL;
+
+ ctx = attr->private;
+ mr = &ctx->tm->mrs[attr - ctx->mrs];
+
+ rc = down_write_killable(&ctx->rwsem);
+ if (rc)
+ return rc;
+
+ rc = ctx->tm->write(ctx->tm, mr, buffer);

There needs to be explicit ABI and driver-api documentation here for what are the
allowed error codes that ->write() can return so as not to be confused
with EINVAL or EINTR arising from user error or interrupt.

I understand your point. But different CC archs may have arch specific reasons for failures. It'd be hard to whitelist all the allowed errors; while blacklisting EINVAL may make more sense, as users have no control over the input (hence cannot provide invalid input) to arch specific write/extend functions. I'll add to the description of ->write() in its kernel-doc comments.

[...]

+/**
+ * tsm_mr_create_attribute_group() - creates an attribute group for measurement
+ * registers
+ * @tm: pointer to &struct tsm_measurements containing the MR definitions.
+ *
+ * This function creates attributes corresponding to the MR definitions
+ * provided by @tm->mrs.
+ *
+ * The created attributes will reference @tm and its members. The caller must
+ * not free @tm until after tsm_mr_free_attribute_group() is called.
+ *
+ * Context: Process context. May sleep due to memory allocation.
+ *
+ * Return:
+ * * On success, the pointer to a an attribute group is returned; otherwise
+ * * %-EINVAL - Invalid MR definitions.
+ * * %-ENOMEM - Out of memory.
+ */
+const struct attribute_group *__must_check

No need to mark this function as __must_check. That attribute is
typically reserved for core-apis.

Will remove.

+tsm_mr_create_attribute_group(const struct tsm_measurements *tm)
+{
+ if (!tm->mrs)
+ return ERR_PTR(-EINVAL);

If you're going to check for !tm->mrs, might as well also check for !tm.

Good catch! Will change.

+
+ /* aggregated length of all MR names */
+ size_t nlen = 0;

Typically the only exceptions for not declaring variables at the top of
a function are for "for ()" loops and scope-based cleanup.

Will move it to the top.

+
+ for (size_t i = 0; i < tm->nr_mrs; ++i) {
+ if ((tm->mrs[i].mr_flags & TSM_MR_F_LIVE) && !tm->refresh)
+ return ERR_PTR(-EINVAL);
+
+ if ((tm->mrs[i].mr_flags & TSM_MR_F_WRITABLE) && !tm->write)
+ return ERR_PTR(-EINVAL);
+
+ if (tm->mrs[i].mr_flags & TSM_MR_F_NOHASH)
+ continue;
+
+ if (WARN_ON(tm->mrs[i].mr_hash >= HASH_ALGO__LAST))

Why potentially crash the kernel here? EINVAL should be sufficient.

Agreed! Will change.

+ return ERR_PTR(-EINVAL);
+
+ /* MR sysfs attribute names have the form of MRNAME:HASH */
+ nlen += strlen(tm->mrs[i].mr_name) + 1 +
+ strlen(hash_algo_name[tm->mrs[i].mr_hash]) + 1;
+ }
+
+ /*
+ * @bas and the MR name strings are combined into a single allocation
+ * so that we don't have to free MR names one-by-one in
+ * tsm_mr_free_attribute_group()
+ */
+ struct bin_attribute **bas __free(kfree) =
+ kzalloc(sizeof(*bas) * (tm->nr_mrs + 1) + nlen, GFP_KERNEL);
+ struct tm_context *ctx __free(kfree) =
+ kzalloc(struct_size(ctx, mrs, tm->nr_mrs), GFP_KERNEL);
+ char *name, *end;
+
+ if (!ctx || !bas)
+ return ERR_PTR(-ENOMEM);
+
+ /* @bas is followed immediately by MR name strings */
+ name = (char *)&bas[tm->nr_mrs + 1];

I looked for a helper macro for a "buffer at the end of a structure",
but could not immediately find one. It feels like something Linux should
already have.

Given a pointer some_struct_p, the end of it will be (some_struct_p + 1) or &some_struct_p[1]. It'd be more readable to be wrapped by a macro for sure.

+ end = name + nlen;
+
+ for (size_t i = 0; i < tm->nr_mrs; ++i) {
+ bas[i] = &ctx->mrs[i];
+ sysfs_bin_attr_init(bas[i]);
+
+ if (tm->mrs[i].mr_flags & TSM_MR_F_NOHASH)
+ bas[i]->attr.name = tm->mrs[i].mr_name;
+ else if (name < end) {
+ bas[i]->attr.name = name;
+ name += snprintf(name, end - name, "%s:%s",
+ tm->mrs[i].mr_name,
+ hash_algo_name[tm->mrs[i].mr_hash]);
+ ++name;
+ } else
+ return ERR_PTR(-EINVAL);
+
+ /* check for duplicated MR definitions */
+ for (size_t j = 0; j < i; ++j)
+ if (!strcmp(bas[i]->attr.name, bas[j]->attr.name))
+ return ERR_PTR(-EINVAL);
+
+ if (tm->mrs[i].mr_flags & TSM_MR_F_READABLE) {
+ bas[i]->attr.mode |= 0444;
+ bas[i]->read_new = tm_digest_read;
+ }
+
+ if (tm->mrs[i].mr_flags & TSM_MR_F_WRITABLE) {
+ bas[i]->attr.mode |= 0220;

Typical expectation for writable attributes is 0200.

Will change.

[...]

+/**
+ * tsm_mr_free_attribute_group() - frees the attribute group returned by
+ * tsm_mr_create_attribute_group()
+ * @attr_grp: attribute group returned by tsm_mr_create_attribute_group()
+ *
+ * Context: Process context.
+ */
+void tsm_mr_free_attribute_group(const struct attribute_group *attr_grp)
+{

Related to the removal of __must_check add safety here for cases where
someone passes in an ERR_PTR():

if (IS_ERR_OR_NULL(attr_grp)
return;

This also makes the function amenable to scope-based cleanup.

Will change.

[...]

+/**
+ * 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 one or more flags, detailed below
+ * @mr_hash: optional hash identifier defined in include/uapi/linux/hash_info.h.
+ *
+ * A CC guest driver encloses an array of this structure in struct
+ * tsm_measurements to detail the measurement facility supported by the
+ * underlying CC hardware.
+ *
+ * @mr_name and @mr_value must stay valid until this structure is no longer in
+ * use.
+ *
+ * @mr_flags is the bitwise-OR of zero or more of the flags below.
+ *
+ * * %TSM_MR_F_READABLE - the sysfs attribute corresponding to this MR is readable.
+ * * %TSM_MR_F_WRITABLE - the sysfs attribute corresponding to this MR is writable.
+ * * %TSM_MR_F_LIVE - this MR's value may differ from the last value written, so
+ * must be read back from the underlying CC hardware/firmware.

Maybe use the word "extend" somewhere in this description for clarity.

Will clarify.

[...]

+struct tsm_measurements {
+ const char *name;
+ const struct tsm_measurement_register *mrs __counted_by(nr_mrs);

I had assumed that __counted_by() is only for inline flexible arrays,
not out-of-line dynamically allocated arrays. Are you sure this does
what you expect?

Thanks for pointing this out! I misunderstood __counted_by, and will remove it.

-Cedric

Next message: Longfang Liu: "[PATCH v7 5/6] hisi_acc_vfio_pci: bugfix live migration function without VF device driver"
Previous message: Longfang Liu: "[PATCH v7 4/6] hisi_acc_vfio_pci: bugfix the problem of uninstalling driver"
In reply to: Dan Williams: "Re: [PATCH v3 1/5] tsm-mr: Add TVM Measurement Register support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]