| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6813ee10d6621_1d6a29436@dwillia2-xfh.jf.intel.com.notmuch>
Date: Thu, 1 May 2025 14:56:32 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: Cedric Xing <cedric.xing@...el.com>, Dan Williams
<dan.j.williams@...el.com>, "Kirill A. Shutemov"
<kirill.shutemov@...ux.intel.com>, Dave Hansen <dave.hansen@...ux.intel.com>,
Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
Borislav Petkov <bp@...en8.de>, <x86@...nel.org>, "H. Peter Anvin"
<hpa@...or.com>
CC: <linux-kernel@...r.kernel.org>, <linux-coco@...ts.linux.dev>, "Dionna
Amalie Glaze" <dionnaglaze@...gle.com>, Guorui Yu
<guorui.yu@...ux.alibaba.com>, James Bottomley
<James.Bottomley@...senpartnership.com>, Dan Middleton
<dan.middleton@...ux.intel.com>, Mikko Ylinen <mikko.ylinen@...ux.intel.com>,
Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@...ux.intel.com>,
Cedric Xing <cedric.xing@...el.com>
Subject: Re: [PATCH v5 5/5] virt: tdx-guest: Expose TDX MRs as sysfs
attributes
Cedric Xing wrote:
> 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 <cedric.xing@...el.com>
> ---
> .../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@...ts.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.
[..]
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fd5bbf78ec8b..3afbf6c310a7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -26284,6 +26284,7 @@ L: x86@...nel.org
> L: linux-coco@...ts.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.h
> diff --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 attestation
> diff --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.
Powered by blists - more mailing lists