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

[Sathyanarayanan Kuppuswamy]

On 9/12/22 3:22 PM, Kirill A . Shutemov wrote:
> 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?

Agree. I will split it into two checks. One for spec related
checks and another for ABI validation.

> 
> !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.

We don't have to allocate kernel memory if we check it here. But I am not against
letting copy_from/to_user() handle it. I will remove the NULL check.

> 
>> +
>> +	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?

User space has no need to know about the error code. In both error cases, if user
wants report, request has to re-submitted. So there is no use in translating
the error codes.

> 
> 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?

We don't have to protect against it here. It is a blocking call. So if user
makes a parallel request, we will wait for TDX Module to service them
in sequence.

> 
>> +		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?

It is the recommended return code for invalid IOCTL commands.

https://www.kernel.org/doc/html/latest/driver-api/ioctl.html

> 
>> +
>> +	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;
>> +}
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer