[v17,1/3] x86/tdx: Add a wrapper to get TDREPORT from the TDX Module

  To support TDX attestation, the TDX guest driver exposes an IOCTL
interface to allow userspace to get the TDREPORT from the TDX module
via TDG.MR.TDREPORT TDCALL.

In order to get the TDREPORT in the TDX guest driver, instead of using
a low level function like __tdx_module_call(), add a
tdx_mcall_get_report() wrapper function to handle it.

This is a preparatory patch for adding attestation support.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---

Changes since v16
 * Added invalid operand error code support.
 * Removed subtype param in tdx_mcall_get_report().

Changes since v15:
 * None

Changes since v14:
 * Instead of exporting __tdx_module_call(), added a new wrapper.
 * Rebased on top of v6.1-rc1

 arch/x86/coco/tdx/tdx.c    | 38 ++++++++++++++++++++++++++++++++++++++
 arch/x86/include/asm/tdx.h |  2 ++
 2 files changed, 40 insertions(+)
  

On Thu, Nov 03, 2022 at 08:23:53PM -0700, Kuppuswamy Sathyanarayanan wrote:
> To support TDX attestation, the TDX guest driver exposes an IOCTL
> interface to allow userspace to get the TDREPORT from the TDX module
> via TDG.MR.TDREPORT TDCALL.
> 
> In order to get the TDREPORT in the TDX guest driver, instead of using
> a low level function like __tdx_module_call(), add a
> tdx_mcall_get_report() wrapper function to handle it.
> 
> This is a preparatory patch for adding attestation support.
> 
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
> 
> Changes since v16
>  * Added invalid operand error code support.
>  * Removed subtype param in tdx_mcall_get_report().
> 
> Changes since v15:
>  * None
> 
> Changes since v14:
>  * Instead of exporting __tdx_module_call(), added a new wrapper.
>  * Rebased on top of v6.1-rc1
> 
>  arch/x86/coco/tdx/tdx.c    | 38 ++++++++++++++++++++++++++++++++++++++
>  arch/x86/include/asm/tdx.h |  2 ++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 928dcf7a20d9..17cf2e9d5849 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -5,6 +5,8 @@
>  #define pr_fmt(fmt)     "tdx: " fmt
>  
>  #include <linux/cpufeature.h>
> +#include <linux/export.h>
> +#include <linux/io.h>
>  #include <asm/coco.h>
>  #include <asm/tdx.h>
>  #include <asm/vmx.h>
> @@ -15,6 +17,7 @@
>  /* TDX module Call Leaf IDs */
>  #define TDX_GET_INFO			1
>  #define TDX_GET_VEINFO			3
> +#define TDX_GET_REPORT			4
>  #define TDX_ACCEPT_PAGE			6
>  
>  /* TDX hypercall Leaf IDs */
> @@ -34,6 +37,10 @@
>  #define VE_GET_PORT_NUM(e)	((e) >> 16)
>  #define VE_IS_IO_STRING(e)	((e) & BIT(4))
>  
> +/* TDX Module call error codes */
> +#define TDCALL_RETURN_CODE(a)	((a) >> 32)
> +#define TDCALL_INVALID_OPERAND	0xc0000100
> +
>  /*
>   * Wrapper for standard use of __tdx_hypercall with no output aside from
>   * return code.
> @@ -98,6 +105,37 @@ static inline void tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
>  		panic("TDCALL %lld failed (Buggy TDX module!)\n", fn);
>  }
>  
> +/**
> + * tdx_mcall_get_report() - Wrapper for TDG.MR.REPORT TDCALL.
> + * @reportdata: Address of the input buffer which contains
> + *              user-defined REPORTDATA to be included into
> + *              TDREPORT.
> + * @tdreport: Address of the output buffer to store TDREPORT.
> + *
> + * Generate TDREPORT using "TDG.MR.REPORT" TDCALL. Refer to section
> + * titled "TDG.MR.REPORT leaf" in the TDX Module 1.0 specification
> + * for detailed information. It is used in the TDX guest driver
> + * module to get the TDREPORT.
> + *
> + * Return 0 on success, -EINVAL for invalid operands, or -EIO on
> + * other TDCALL failures.
> + */
> +int tdx_mcall_get_report(u8 *reportdata, u8 *tdreport)
> +{
> +	u64 ret;
> +
> +	ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(tdreport),
> +				virt_to_phys(reportdata), 0, 0, NULL);
> +	if (ret) {
> +		if (TDCALL_RETURN_CODE(ret) == TDCALL_INVALID_OPERAND)
> +			return -EINVAL;
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(tdx_mcall_get_report);
> +
>  static u64 get_cc_mask(void)
>  {
>  	struct tdx_module_output out;
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 020c81a7c729..eef9c0b7880e 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -67,6 +67,8 @@ void tdx_safe_halt(void);
>  
>  bool tdx_early_handle_ve(struct pt_regs *regs);
>  
> +int tdx_mcall_get_report(u8 *reportdata, u8 *tdreport);
> +
>  #else
>  
>  static inline void tdx_early_init(void) { };
> -- 
> 2.34.1
> 
> 

Acked-by: Wander Lairson Costa <wander@redhat.com>
  

On 11/3/22 20:23, Kuppuswamy Sathyanarayanan wrote:
> To support TDX attestation, the TDX guest driver exposes an IOCTL
> interface to allow userspace to get the TDREPORT from the TDX module
> via TDG.MR.TDREPORT TDCALL.

This all acts and is named like this is *THE* way to do a TD report.
This is the only type of TD report.

Is it?

If so, why is there a subtype in the TDX module ABI?  It's easy to miss
in the kernel code, btw:

> +int tdx_mcall_get_report(u8 *reportdata, u8 *tdreport)
> +{
> +	u64 ret;
> +
> +	ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(tdreport),
> +				virt_to_phys(reportdata), 0, 0, NULL);

					     subtype here ^

mixed in next to another magic 0.

> +	if (ret) {
> +		if (TDCALL_RETURN_CODE(ret) == TDCALL_INVALID_OPERAND)
> +			return -EINVAL;
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(tdx_mcall_get_report);

What happens to this interface when subtype 1 is added?

TDX_CMD_GET_REPORT can only get subtype 0.  So, we'll have, what, a new
ioctl()?  TDX_CMD_GET_REPORT_SUBTYPE1?

This is why I was pushing for a more generic ABI that would actually
work for more than one subtype.  Other folks thought that was a bad
idea.  I can live with that.  But, what I can't live with is just
pretending that this is the one and only forever "tdreport" interface.

This is *NOT* "a wrapper to get TDREPORT from the TDX Module", this is
at best "a wrapper to get TDREPORT sub type 0 from the TDX Module".

It also occurs to me that "sub type 0" could use an actual name.  Could
we give it one, please?
  

Hi Dave,

On 11/11/22 10:35 AM, Dave Hansen wrote:
> This is *NOT* "a wrapper to get TDREPORT from the TDX Module", this is
> at best "a wrapper to get TDREPORT sub type 0 from the TDX Module".

In both the commit log and the comments, I can highlight the "subtype 0"
information. Will that work for you, or do you prefer that this wrapper
take the "subtype" option as argument and we pass 0 for the subtype value
from the TDX guest driver?

> 
> It also occurs to me that "sub type 0" could use an actual name.  Could
> we give it one, please?

Although the subtype option is mentioned in the TDX Module spec, it is not
currently used (it expects this value to be zero), and the spec also does
not explain why this option is required. According to TDX architects, this
option was primarily added to handle any future requirements that may arise
that require additional information to be added to the TDREPORT. However,
they do not currently have any valid use cases for it. So the current
version can only be described as "Type-0." Once a new use case for Subtype 1
is defined, we may be able to come up with a suitable name. Are you okay
with calling it "Type-0" for the time being?
  

On 11/14/22 16:33, Sathyanarayanan Kuppuswamy wrote:
> On 11/11/22 10:35 AM, Dave Hansen wrote:
>> This is *NOT* "a wrapper to get TDREPORT from the TDX Module", this is
>> at best "a wrapper to get TDREPORT sub type 0 from the TDX Module".
> 
> In both the commit log and the comments, I can highlight the "subtype 0"
> information. Will that work for you, or do you prefer that this wrapper
> take the "subtype" option as argument and we pass 0 for the subtype value
> from the TDX guest driver?

I actually think it's a *lot* more clear if the User<->Kernel ABI just
takes the subtype.  But, I also heard Greg's concerns about making the
ABI _too_ open-ended.

So, I really don't care.  Just make it clear that, as is, this ABI is
not the "TDREPORT ABI".

>> It also occurs to me that "sub type 0" could use an actual name.  Could
>> we give it one, please?
> 
> Although the subtype option is mentioned in the TDX Module spec, it is not
> currently used (it expects this value to be zero), and the spec also does
> not explain why this option is required. According to TDX architects, this
> option was primarily added to handle any future requirements that may arise
> that require additional information to be added to the TDREPORT. However,
> they do not currently have any valid use cases for it. So the current
> version can only be described as "Type-0." Once a new use case for Subtype 1
> is defined, we may be able to come up with a suitable name. Are you okay
> with calling it "Type-0" for the time being?

That sounds like a cop out to me.  I'd really appreciate some effort on
your part to look deeply into the problem.

The blob that the kernel is passing back and forth here _has_ content.
I guess it's somewhat hard to name because it's got a bunch of inputs
(ATTRIBUTES, XFAM, MRTD, MRCONFIGID, MROWNER, MROWNERCONFIG and RTMRs)
and a fixed hash algorithm (SHA-384).

Any time that those inputs change or, for instance, the hash algorithm
changes, it would need a new subtype.  Right?

I guess we can't call "subtype 0" TDREPORT_SHA384 because "subtype 1"
might still use SHA-384, but have the set of inputs change.

But, it'll also get maddeningly inconsistent if we have a "TDREPORT"
ioctl() that does "subtype 0" and "TDREPORT1" that does "subtype 1".

So, let's at *least* call this thing "TDREPORT0" in the ABI, along with
a description of why we're numbering it that way as opposed to taking
'subtype' as a numeric ioctl() argument.

Any better ideas?
  

Hi Dave,

On 11/14/22 4:54 PM, Dave Hansen wrote:
>> In both the commit log and the comments, I can highlight the "subtype 0"
>> information. Will that work for you, or do you prefer that this wrapper
>> take the "subtype" option as argument and we pass 0 for the subtype value
>> from the TDX guest driver?
> I actually think it's a *lot* more clear if the User<->Kernel ABI just
> takes the subtype.  But, I also heard Greg's concerns about making the
> ABI _too_ open-ended.
> 
> So, I really don't care.  Just make it clear that, as is, this ABI is
> not the "TDREPORT ABI".
> 

Are you fine with the following version?

+/* TDX Module call error codes */
+#define TDCALL_RETURN_CODE(a)  ((a) >> 32)
+#define TDCALL_INVALID_OPERAND 0xc0000100
+
+#define TDREPORT_SUBTYPE_0     0
+ 
+/**
+ * tdx_mcall_get_report0() - Wrapper to get TDREPORT0 (a.k.a. TDREPORT
+ *                           subtype 0) using TDG.MR.REPORT TDCALL.
+ * @reportdata: Address of the input buffer which contains user-defined
+ *              REPORTDATA to be included into TDREPORT.
+ * @tdreport: Address of the output buffer to store TDREPORT.
+ *
+ * Refer to section titled "TDG.MR.REPORT leaf" in the TDX Module
+ * v1.0 specification for more information on TDG.MR.REPORT TDCALL.
+ * It is used in the TDX guest driver module to get the TDREPORT0.
+ *
+ * Return 0 on success, -EINVAL for invalid operands, or -EIO on
+ * other TDCALL failures.
+ */
+int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport)
+{
+       u64 ret;
+
+       ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(tdreport),
+                               virt_to_phys(reportdata), TDREPORT_SUBTYPE_0,
+                               0, NULL);
+       if (ret) {
+               if (TDCALL_RETURN_CODE(ret) == TDCALL_INVALID_OPERAND)
+                       return -EINVAL;
+               return -EIO;
+       }
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(tdx_mcall_get_report0);