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