| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 12 Sep 2022 16:00:15 -0700
From: Sathyanarayanan Kuppuswamy
<sathyanarayanan.kuppuswamy@...ux.intel.com>
To: "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
Shuah Khan <shuah@...nel.org>,
"H . Peter Anvin" <hpa@...or.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Tony Luck <tony.luck@...el.com>,
Andi Kleen <ak@...ux.intel.com>,
Kai Huang <kai.huang@...el.com>,
Wander Lairson Costa <wander@...hat.com>,
Isaku Yamahata <isaku.yamahata@...il.com>,
marcelo.cerri@...onical.com, tim.gardner@...onical.com,
khalid.elmously@...onical.com, philip.cox@...onical.com,
linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
linux-doc@...r.kernel.org
Subject: Re: [PATCH v13 1/3] x86/tdx: Add TDX Guest attestation interface
driver
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
Powered by blists - more mailing lists