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