Re: [PATCH v5 5/5] virt: tdx-guest: Expose TDX MRs as sysfs attributes
From: Dan Williams <hidden>
Date: 2025-05-01 21:56:42
Also in:
lkml
Cedric Xing wrote:
quoted hunk ↗ jump to hunk
Expose the most commonly used TDX MRs (Measurement Registers) as sysfs attributes. Use the ioctl() interface of /dev/tdx_guest to request a full TDREPORT for access to other TD measurements. Directory structure of TDX MRs inside a TDVM is as follows: /sys/class/misc/tdx_guest └── mr ├── mrconfigid ├── mrowner ├── mrownerconfig ├── mrtd:sha384 ├── rtmr0:sha384 ├── rtmr1:sha384 ├── rtmr2:sha384 └── rtmr3:sha384 Read the file/attribute to retrieve the current value of an MR. Write to the file/attribute (if writable) to extend the corresponding RTMR. Refer to Documentation/ABI/testing/sysfs-devices-virtual-misc-tdx_guest for more information. Signed-off-by: Cedric Xing <redacted> --- .../testing/sysfs-devices-virtual-misc-tdx_guest | 60 +++++ MAINTAINERS | 1 + drivers/virt/coco/tdx-guest/Kconfig | 1 + drivers/virt/coco/tdx-guest/tdx-guest.c | 291 +++++++++++++-------- 4 files changed, 250 insertions(+), 103 deletions(-)diff --git a/Documentation/ABI/testing/sysfs-devices-virtual-misc-tdx_guest b/Documentation/ABI/testing/sysfs-devices-virtual-misc-tdx_guest new file mode 100644 index 000000000000..fb7b27ae0b74 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-devices-virtual-misc-tdx_guest@@ -0,0 +1,60 @@ +What: /sys/devices/virtual/misc/tdx_guest/mr/MRNAME[:HASH]
I missed this on patch1, but I am still not a fan of this attribute directory being called "mr". Please make it "measurement_registers" or "measurements" to remove ambiguity.
+Date: April, 2025 +KernelVersion: v6.16 +Contact: linux-coco@lists.linux.dev +Description: + Value of a TDX measurement register (MR). MRNAME and HASH above + are placeholders. The optional suffix :HASH is used for MRs + that have associated hash algorithms. See below for a complete + list of TDX MRs exposed via sysfs. Refer to Intel TDX Module + ABI Specification for the definition of TDREPORT and the full + list of TDX measurements. + + Intel TDX Module ABI Specification can be found at: + https://www.intel.com/content/www/us/en/developer/tools/trust-domain-extensions/documentation.html#architecture
Documentation indeed looks better now. I had also been wanting this ABI documentation to refer to a generic description of how measurement-register sysfs files behave. I think you can achieve that simply with a link back to the generated driver-api kdoc: https://docs.kernel.org/driver-api/coco/measurement-registers.html Of course, that link will start resolving once this is upstream. [..]
quoted hunk ↗ jump to hunk
diff --git a/MAINTAINERS b/MAINTAINERS index fd5bbf78ec8b..3afbf6c310a7 100644 --- a/MAINTAINERS +++ b/MAINTAINERS@@ -26284,6 +26284,7 @@ L: x86@kernel.org L: linux-coco@lists.linux.dev S: Supported T: git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/tdx +F: Documentation/ABI/testing/sysfs-devices-virtual-misc-tdx_guest F: arch/x86/boot/compressed/tdx* F: arch/x86/coco/tdx/ F: arch/x86/include/asm/shared/tdx.hdiff --git a/drivers/virt/coco/tdx-guest/Kconfig b/drivers/virt/coco/tdx-guest/Kconfig index 22dd59e19431..dbbdc14383b1 100644 --- a/drivers/virt/coco/tdx-guest/Kconfig +++ b/drivers/virt/coco/tdx-guest/Kconfig@@ -2,6 +2,7 @@ config TDX_GUEST_DRIVER tristate "TDX Guest driver" depends on INTEL_TDX_GUEST select TSM_REPORTS + select TSM_MEASUREMENTS help The driver provides userspace interface to communicate with the TDX module to request the TDX guest details like attestationdiff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c index 224e7dde9cde..e76810d89e0b 100644 --- a/drivers/virt/coco/tdx-guest/tdx-guest.c +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c@@ -5,6 +5,8 @@ * Copyright (C) 2022 Intel Corporation */ +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + #include <linux/kernel.h> #include <linux/miscdevice.h> #include <linux/mm.h>@@ -15,8 +17,9 @@ #include <linux/set_memory.h> #include <linux/io.h> #include <linux/delay.h> +#include <linux/sockptr.h> #include <linux/tsm.h> -#include <linux/sizes.h> +#include <linux/tsm-mr.h> #include <uapi/linux/tdx-guest.h>@@ -66,39 +69,144 @@ static DEFINE_MUTEX(quote_lock); */ static u32 getquote_timeout = 30; -static long tdx_get_report0(struct tdx_report_req __user *req) +/* Buffers for TDREPORT and RTMR extension */ +static u8 *tdx_report_buf, *tdx_extend_buf; + +/* Lock to serialize TDG.MR.REPORT & TDG.MR.RTMR.EXTEND requests */ +static DEFINE_MUTEX(mr_lock); + +/* TDREPORT fields */ +enum { + TDREPORT_reportdata = 128, + TDREPORT_tee_tcb_info = 256, + TDREPORT_tdinfo = TDREPORT_tee_tcb_info + 256, + TDREPORT_attributes = TDREPORT_tdinfo, + TDREPORT_xfam = TDREPORT_attributes + sizeof(u64), + TDREPORT_mrtd = TDREPORT_xfam + sizeof(u64), + TDREPORT_mrconfigid = TDREPORT_mrtd + SHA384_DIGEST_SIZE, + TDREPORT_mrowner = TDREPORT_mrconfigid + SHA384_DIGEST_SIZE, + TDREPORT_mrownerconfig = TDREPORT_mrowner + SHA384_DIGEST_SIZE, + TDREPORT_rtmr0 = TDREPORT_mrownerconfig + SHA384_DIGEST_SIZE, + TDREPORT_rtmr1 = TDREPORT_rtmr0 + SHA384_DIGEST_SIZE, + TDREPORT_rtmr2 = TDREPORT_rtmr1 + SHA384_DIGEST_SIZE, + TDREPORT_rtmr3 = TDREPORT_rtmr2 + SHA384_DIGEST_SIZE, + TDREPORT_servtd_hash = TDREPORT_rtmr3 + SHA384_DIGEST_SIZE, +}; + +static inline int tdx_mr_report_locked(sockptr_t data, sockptr_t tdreport) { - u8 *reportdata, *tdreport; - long ret; + scoped_cond_guard(mutex_intr, return -EINTR, &mr_lock) {
Why a new @mr_lock when @quote_lock exists? Are they not protecting the same things?
+ u8 *p = tdx_report_buf + TDREPORT_reportdata;
+ int ret;
- reportdata = kmalloc(TDX_REPORTDATA_LEN, GFP_KERNEL);
- if (!reportdata)
- return -ENOMEM;
+ if (!sockptr_is_null(data) &&
+ copy_from_sockptr(p, data, TDX_REPORTDATA_LEN))
+ return -EFAULT;
- tdreport = kzalloc(TDX_REPORT_LEN, GFP_KERNEL);
- if (!tdreport) {
- ret = -ENOMEM;
- goto out;
+ ret = tdx_mcall_get_report0(p, tdx_report_buf);
+ if (WARN(ret, "tdx_mcall_get_report0() failed: %d", ret))
+ return ret;
+
+ if (!sockptr_is_null(tdreport) &&
+ copy_to_sockptr(tdreport, tdx_report_buf, TDX_REPORT_LEN))
+ return -EFAULT;
}
+ return 0;
+}
- if (copy_from_user(reportdata, req->reportdata, TDX_REPORTDATA_LEN)) {
- ret = -EFAULT;
- goto out;
+static inline int tdx_mr_extend_locked(ptrdiff_t mr_ind, const u8 *data)
+{
+ scoped_cond_guard(mutex_intr, return -EINTR, &mr_lock) {
+ int ret;
+
+ memcpy(tdx_extend_buf, data, SHA384_DIGEST_SIZE);
+
+ ret = tdx_mcall_extend_rtmr(mr_ind, tdx_extend_buf);
+ if (WARN(ret, "tdx_mcall_extend_rtmr(%ld) failed: %d", mr_ind, ret))
+ return ret;
}
+ return 0;
+}
Ok, the above hunk is an example that this patch is doing too much at
once and making the conversion unnecessarily difficult to follow.
There are 3 separate topics in this patch that deserve to be split into
3 patches:
1) Convert tdx_guest to cleanup helpers
Now, I do not personally like the fact that conversion to
scoped_cond_guard() causes a bunch of code to be re-indented, that is
messy. One alternative to scoped_cond_guard() that I have been
considering based on feedback Linus gave when that helper was
introduced is instead do something like this:
DEFINE_FREE(mutex_intr_release, struct mutex, if (_T) mutex_unlock(_T))
static struct mutex *mutex_intr_acquire(struct mutex *lock)
{
if (mutex_lock_interruptible(lock))
return lock;
return NULL;
}
...then you can do:
struct mutex *lock __free(mutex_intr_release) = mutex_intr_acquire(&mr_lock);
Where that allows you to not need to re-indent the whole function.
If the lock has limited scope within a function then just move that bit
to a helper function rather than re-indent existing code.
2) Conversion to sockptr. Prepare the report retrieval code to
alternatively store into kernel or user buffers.
3) RTMR support on top of that converted / cleaned-up base. This will be
much easier to read withtout the re-indentation mess and other unrelated code
movement. Just a clean feature addition patch.