Re: [PATCH v13 1/3] x86/tdx: Add TDX Guest attestation interface driver

On Fri, Sep 09, 2022 at 12:27:06PM -0700, Kuppuswamy Sathyanarayanan wrote:

diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c index 928dcf7a20d9..8b5c59110321 100644 --- a/arch/x86/coco/tdx/tdx.c +++ b/arch/x86/coco/tdx/tdx.c @@ -5,16 +5,21 @@ #define pr_fmt(fmt) "tdx: " fmt #include <linux/cpufeature.h> +#include <linux/miscdevice.h> +#include <linux/mm.h> +#include <linux/io.h> #include <asm/coco.h> #include <asm/tdx.h> #include <asm/vmx.h> #include <asm/insn.h> #include <asm/insn-eval.h> #include <asm/pgtable.h> +#include <uapi/asm/tdx.h> /* 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 */ @@ -775,3 +780,113 @@ void __init tdx_early_init(void) pr_info("Guest detected\n"); }

+static long tdx_get_report(void __user *argp) +{

• u8 *reportdata, *tdreport;
• struct tdx_report_req req;
• u8 reserved[7] = {0};
• long ret;
• if (copy_from_user(&req, argp, sizeof(req)))

• return -EFAULT;
  

• /*

• * Per TDX Module 1.0 specification, section titled
  

• * "TDG.MR.REPORT", REPORTDATA length is fixed as
  

• * TDX_REPORTDATA_LEN, TDREPORT length is fixed as
  

• * TDX_REPORT_LEN, and TDREPORT subtype is fixed as
  

• * 0. Also check for valid user pointers and make sure
  

• * reserved entries values are zero.
  

• */
  

• if (!req.reportdata || !req.tdreport || req.subtype ||

• req.rpd_len != TDX_REPORTDATA_LEN ||
  

• req.tdr_len != TDX_REPORT_LEN ||
  

• memcmp(req.reserved, reserved, 7))
  

• return -EINVAL;
  

Maybe make several checks instead of the monstrous one?

!req.reportdata and !req.tdreport look redundant. copy_from/to_user() will catch them (and other bad address cases). And -EFAULT is more appropriate in this case.

• reportdata = kmalloc(req.rpd_len, GFP_KERNEL);
• if (!reportdata)

• return -ENOMEM;
  

• tdreport = kzalloc(req.tdr_len, GFP_KERNEL);
• if (!tdreport) {

• kfree(reportdata);
  

• return -ENOMEM;
  

• }
• if (copy_from_user(reportdata, u64_to_user_ptr(req.reportdata),

• 	   req.rpd_len)) {
  

• ret = -EFAULT;
  

• goto out;
  

• }
• /*

• * Generate TDREPORT using "TDG.MR.REPORT" TDCALL.
  

• *
  

• * Get the TDREPORT using REPORTDATA as input. Refer to
  

• * section 22.3.3 TDG.MR.REPORT leaf in the TDX Module 1.0
  

• * Specification for detailed information.
  

• */
  

• ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(tdreport),

• 		virt_to_phys(reportdata), req.subtype,
  

• 		0, NULL);
  

• if (ret) {

• ret = -EIO;
  

The spec says that it generate an error if invalid operand or busy. Maybe translate the TDX error codes to errnos?

BTW, regarding busy case: do we want to protect against two parallel TDX_GET_REPORT? What happens if we run the second TDX_GET_REPORT when the first hasn't complete?

• goto out;
  

• }
• if (copy_to_user(u64_to_user_ptr(req.tdreport), tdreport, req.tdr_len))

• ret = -EFAULT;
  

+out:

• kfree(reportdata);
• kfree(tdreport);
• return ret;

+} +static long tdx_guest_ioctl(struct file *file, unsigned int cmd,

• 	    unsigned long arg)
  

+{

• void __user *argp = (void __user *)arg;
• long ret = -ENOTTY;

Not a typewriter? Huh?

• switch (cmd) {
• case TDX_CMD_GET_REPORT:

• ret = tdx_get_report(argp);
  

• break;
  

• default:

• pr_debug("cmd %d not supported\n", cmd);
  

• break;
  

• }
• return ret;

+}