Re: [PATCH v9 1/6] x86/tdx: Add TDX Guest attestation interface driver

Sathyanarayanan Kuppuswamy
2022-08-18T14:41:01Z

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