Re: [PATCH v5 5/5] virt: tdx-guest: Expose TDX MRs as sysfs attributes

[Dan Williams <dan.j.williams@intel.com>]

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@intel.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@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.

[..]
> 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.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.