Re: [PATCH v16 2/3] virt: Add TDX guest driver

[Greg Kroah-Hartman]

On Thu, Oct 27, 2022 at 05:28:19PM -0700, Kuppuswamy Sathyanarayanan wrote:
> +	/*
> +	 * Check for valid input values. Per TDX Module 1.0
> +	 * specification, section titled "TDG.MR.REPORT",
> +	 * REPORTDATA length is fixed as TDX_REPORTDATA_LEN,

If this length is fixed, then you can never change it, so why have it at
all?

> +	 * TDREPORT length is fixed as TDX_REPORT_LEN, and
> +	 * TDREPORT subtype is fixed as 0.
> +	 */
> +	if (req.subtype || req.rpd_len != TDX_REPORTDATA_LEN ||
> +	    req.tdr_len != TDX_REPORT_LEN)
> +		return -EINVAL;
> +
> +	if (memchr_inv(req.reserved, 0, sizeof(req.reserved)))
> +		return -EINVAL;
> +
> +	reportdata = kmalloc(req.rpd_len, GFP_KERNEL);
> +	if (!reportdata)
> +		return -ENOMEM;
> +
> +	tdreport = kzalloc(req.tdr_len, GFP_KERNEL);
> +	if (!tdreport) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	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 */
> +	ret = tdx_mcall_get_report(reportdata, tdreport, req.subtype);
> +	if (ret)
> +		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)
> +{
> +	switch (cmd) {
> +	case TDX_CMD_GET_REPORT:
> +		return tdx_get_report((void __user *)arg);

You know the type of this pointer here, why not cast it instead of
having to cast it from void * again?

> +	default:
> +		return -ENOTTY;
> +	}
> +}
> +
> +static const struct file_operations tdx_guest_fops = {
> +	.owner = THIS_MODULE,
> +	.unlocked_ioctl = tdx_guest_ioctl,
> +	.llseek = no_llseek,
> +};
> +
> +static struct miscdevice tdx_misc_dev = {
> +	.name = KBUILD_MODNAME,
> +	.minor = MISC_DYNAMIC_MINOR,
> +	.fops = &tdx_guest_fops,
> +};
> +
> +static const struct x86_cpu_id tdx_guest_ids[] = {
> +	X86_MATCH_FEATURE(X86_FEATURE_TDX_GUEST, NULL),
> +	{}
> +};
> +MODULE_DEVICE_TABLE(x86cpu, tdx_guest_ids);
> +
> +static int __init tdx_guest_init(void)
> +{
> +	if (!x86_match_cpu(tdx_guest_ids))
> +		return -ENODEV;
> +
> +	return misc_register(&tdx_misc_dev);
> +}
> +module_init(tdx_guest_init);
> +
> +static void __exit tdx_guest_exit(void)
> +{
> +	misc_deregister(&tdx_misc_dev);
> +}
> +module_exit(tdx_guest_exit);
> +
> +MODULE_AUTHOR("Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("TDX Guest Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/uapi/linux/tdx-guest.h b/include/uapi/linux/tdx-guest.h
> new file mode 100644
> index 000000000000..29453e6a7ced
> --- /dev/null
> +++ b/include/uapi/linux/tdx-guest.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Userspace interface for TDX guest driver
> + *
> + * Copyright (C) 2022 Intel Corporation
> + */
> +
> +#ifndef _UAPI_LINUX_TDX_GUEST_H_
> +#define _UAPI_LINUX_TDX_GUEST_H_
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +/* Length of the REPORTDATA used in TDG.MR.REPORT TDCALL */
> +#define TDX_REPORTDATA_LEN              64
> +
> +/* Length of TDREPORT used in TDG.MR.REPORT TDCALL */
> +#define TDX_REPORT_LEN                  1024

As these are fixed values, why do you have to say them again in the
structure?

If you ever change them, wonderful, make a new ioctl.  You can't change
this one as userspace would have to also change, there is no way you
could mix/match kernel versions and userspace and not have problems.

So just fix these values, like you have, and remove them from the
structure definition as there's no way you can change them anyway.

> +
> +/**
> + * struct tdx_report_req - Request struct for TDX_CMD_GET_REPORT IOCTL.
> + *
> + * @reportdata: User-defined REPORTDATA to be included into TDREPORT.
> + *              Typically it can be some nonce provided by attestation
> + *              service, so the generated TDREPORT can be uniquely verified.
> + * @tdreport: TDREPORT output from TDCALL[TDG.MR.REPORT].

These are userspace pointers, document them as such please.

> + * @rpd_len: Length of the REPORTDATA (fixed as 64 bytes by the TDX Module
> + *           specification, but a parameter is added to handle future
> + *           extension).

As I say above, this can't be changed, right?

> + * @tdr_len: Length of the TDREPORT (fixed as 1024 bytes by the TDX Module
> + *           specification, but a parameter is added to accommodate future
> + *           extension).

Again, can't be changed.

> + * @subtype: Subtype of TDREPORT (fixed as 0 by the TDX Module specification,
> + *           but added a parameter to handle future extension).

If this too is fixed, you can't change it.

thanks,

greg k-h