[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7e4a4746-8146-b0b2-e0b3-d32960421da6@linux.intel.com>
Date: Thu, 18 Aug 2022 07:40:51 -0700
From: Sathyanarayanan Kuppuswamy
<sathyanarayanan.kuppuswamy@...ux.intel.com>
To: Borislav Petkov <bp@...en8.de>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>,
Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
"H . Peter Anvin" <hpa@...or.com>,
"Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
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
Subject: Re: [PATCH v9 1/6] x86/tdx: Add TDX Guest attestation interface
driver
Hi Boris,
On 8/18/22 7:18 AM, Borislav Petkov wrote:
> On Wed, Jul 27, 2022 at 08:44:15PM -0700, Kuppuswamy Sathyanarayanan wrote:
>
> ...
>
We have a v10 version of this patch. We have dropped GetQuote support as per Dave's
comment. If it is not a problem, for the rest of the patches in this series, please
check v10.
https://lore.kernel.org/lkml/Yu1z0KcU5C2AJO6S@fedora/T/
>> Operations like getting TDREPORT or Quote generation involves sending
>> a blob of data as input and getting another blob of data as output. It
>> was considered to use a sysfs interface for this, but it doesn't fit
>> well into the standard sysfs model for configuring values. It would be
>> possible to do read/write on files, but it would need multiple file
>> descriptors, which would be somewhat messy. IOCTLs seems to be the best
>> fitting and simplest model for this use case. This is similar to AMD
>> SEV platform, which also uses IOCTL interface to support attestation.
>
> So the gist of this whole commit message - with the TD<->TDX
> nomenclature fixed - needs to go to Documentation/x86/tdx.rst.
Ok. I will add it in next version.
>
>> diff --git a/arch/x86/coco/tdx/attest.c b/arch/x86/coco/tdx/attest.c
>> new file mode 100644
>> index 000000000000..46a2f3612753
>> --- /dev/null
>> +++ b/arch/x86/coco/tdx/attest.c
>> @@ -0,0 +1,81 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * attest.c - TDX attestation feature support.
>
> s/feature //
Ok.
>
>> + *
>> + * Implements attestation related IOCTL handlers.
>> + *
>> + * Copyright (C) 2022 Intel Corporation
>> + *
>> + */
>> +
>> +#include <linux/mm.h>
>> +#include <linux/io.h>
>> +#include <asm/tdx.h>
>> +
>> +#include "tdx.h"
>> +
>> +/* TDREPORT module call leaf ID */
>> +#define TDX_GET_REPORT 4
>
> All TDX leaf definitions go to arch/x86/include/asm/shared/tdx.h, for
> example.
>
> Not spread around the tree. There are some in arch/x86/coco/tdx/tdx.c
> too.
>
> In a pre-patch: please pick a fitting header, move them there and keep
> them all there.
Sure. Will move it.
>
>> +long tdx_get_report(void __user *argp)
>> +{
>> + u8 *reportdata = NULL, *tdreport = NULL;
>> + struct tdx_report_req req;
>> + long ret;
>> +
>> + /* Copy request struct from the user buffer */
>
> Useless comment.
Ok. I will remove it in next version.
>
>> + if (copy_from_user(&req, argp, sizeof(req)))
>> + return -EFAULT;
>> +
>> + /*
>> + * Per TDX Module 1.0 specification, section titled
>> + * "TDG.MR.REPORT", REPORTDATA and TDREPORT length
>> + * is fixed as TDX_REPORTDATA_LEN and TDX_REPORT_LEN.
>> + */
>> + if (req.rpd_len != TDX_REPORTDATA_LEN || req.tdr_len != TDX_REPORT_LEN)
>> + return -EINVAL;
>> +
>> + /* Allocate kernel buffers for REPORTDATA and TDREPORT */
>> + reportdata = kzalloc(req.rpd_len, GFP_KERNEL);
>> + if (!reportdata) {
>> + ret = -ENOMEM;
>> + goto report_failed;
>> + }
>> +
>> + tdreport = kzalloc(req.tdr_len, GFP_KERNEL);
>> + if (!tdreport) {
>> + ret = -ENOMEM;
>> + goto report_failed;
>> + }
>> +
>> +
>> + /* Copy REPORTDATA from user to kernel buffer */
>
> Useless comment.
Ok. I will remove it in next version.
>
>> + if (copy_from_user(reportdata, (void *)req.reportdata, req.rpd_len)) {
>
> You're trusting a user pointer without any checks?
>
> I guess there's not a lot you can check besides the length with you do.
> If there are sanity checks you can do, though, do them here.
I will add NULL pointer check and a subtype validity check there.
>
>> + ret = -EFAULT;
>> + goto report_failed;
>> + }
>> +
>> + /*
>> + * 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,
>
> That subtype you're not checking either.
>
> Where's the paranoia?!
>
>> + 0, NULL);
>> + if (ret) {
>> + ret = -EIO;
>> + goto report_failed;
>> + }
>> +
>> + /* Copy TDREPORT data back to the user buffer */
>
> Another useless comment.
Will remove it.
>
>> + if (copy_to_user((void *)req.tdreport, tdreport, req.tdr_len))
>> + ret = -EFAULT;
>> +
>> +report_failed:
>> + kfree(reportdata);
>> + kfree(tdreport);
>> + return ret;
>> +}
>> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
>> index 928dcf7a20d9..205f98f42da8 100644
>> --- a/arch/x86/coco/tdx/tdx.c
>> +++ b/arch/x86/coco/tdx/tdx.c
>> @@ -5,6 +5,9 @@
>> #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>
>> @@ -12,6 +15,8 @@
>> #include <asm/insn-eval.h>
>> #include <asm/pgtable.h>
>>
>> +#include "tdx.h"
>> +
>> /* TDX module Call Leaf IDs */
>> #define TDX_GET_INFO 1
>> #define TDX_GET_VEINFO 3
>> @@ -34,6 +39,10 @@
>> #define VE_GET_PORT_NUM(e) ((e) >> 16)
>> #define VE_IS_IO_STRING(e) ((e) & BIT(4))
>>
>> +#define DRIVER_NAME "tdx-guest"
>
> Just "tdx". When you add another driver, then you can disambiguate.
Agree.
>
>> +static struct miscdevice tdx_misc_dev;
>> +
>> /*
>> * Wrapper for standard use of __tdx_hypercall with no output aside from
>> * return code.
>> @@ -775,3 +784,49 @@ void __init tdx_early_init(void)
>>
>> pr_info("Guest detected\n");
>> }
>> +
>> +static long tdx_guest_ioctl(struct file *file, unsigned int cmd,
>> + unsigned long arg)
>> +{
>> + void __user *argp = (void __user *)arg;
>> + long ret = -EINVAL;
>> +
>> + 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;
>> +}
>> +
>> +static const struct file_operations tdx_guest_fops = {
>> + .owner = THIS_MODULE,
>> + .unlocked_ioctl = tdx_guest_ioctl,
>> + .llseek = no_llseek,
>> +};
>> +
>> +static int __init tdx_guest_init(void)
>> +{
>> + int ret;
>> +
>> + /* Make sure we are in a valid TDX platform */
>
> More useless comments.
>
> When you type comments, pls stop and think whether it even makes sense
> to add them or the code you're commenting is actually clear from the
> function naming and the given parameters and the position in the
> function..., from all of it, that it is pretty clear what happens.
Ok. I will check and remove obvious comments.
>
>> + if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
>> + return -EIO;
>> +
>> + tdx_misc_dev.name = DRIVER_NAME;
>> + tdx_misc_dev.minor = MISC_DYNAMIC_MINOR;
>> + tdx_misc_dev.fops = &tdx_guest_fops;
>> +
>> + ret = misc_register(&tdx_misc_dev);
>> + if (ret) {
>> + pr_err("misc device registration failed\n");
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +device_initcall(tdx_guest_init)
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
Powered by blists - more mailing lists