Message-ID: <Yv5KNyX992ddvVtD@zn.tnic>
Date:   Thu, 18 Aug 2022 16:18:31 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>
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

On Wed, Jul 27, 2022 at 08:44:15PM -0700, Kuppuswamy Sathyanarayanan wrote:

...

> 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.

> 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 //

> + *
> + * 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.

> +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.

> +	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.

> +	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.

> +		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.

> +	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.

> +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.

> +	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)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Powered by blists - more mailing lists