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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Wed, Oct 19, 2022 at 09:58:27PM -0700, Kuppuswamy Sathyanarayanan wrote:
> +static long tdx_get_report(void __user *argp)
> +{
> +	u8 *reportdata, *tdreport;
> +	struct tdx_report_req req;
> +	long ret;
> +
> +	if (copy_from_user(&req, argp, sizeof(req)))
> +		return -EFAULT;
> +
> +	/*
> +	 * Per TDX Module 1.0 specification, section titled
> +	 * "TDG.MR.REPORT", REPORTDATA length is fixed as
> +	 * TDX_REPORTDATA_LEN, 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) {
> +		pr_err("TDX_CMD_GET_REPORT: invalid req: subtype:%u rpd_len:%u tdr_len:%u\n",
> +		       req.subtype, req.rpd_len, req.tdr_len);

You are allowing userspace to spam the kernel logs, please do not do
that.

Also, you have a real device here, use it and call dev_*() instead of
pr_*().  Your code should not have any pr_* calls.


> +		return -EINVAL;
> +	}
> +
> +	if (memchr_inv(req.reserved, 0, sizeof(req.reserved))) {
> +		pr_err("TDX_CMD_GET_REPORT: Non zero value in reserved field\n");
> +		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) {
> +		pr_err("TDX_CMD_GET_REPORT: TDCALL failed\n");
> +		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);
> +	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 int __init tdx_guest_init(void)
> +{
> +	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> +		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);
> +
> +#ifdef MODULE
> +static const struct x86_cpu_id tdx_guest_ids[] = {
> +	X86_MATCH_FEATURE(X86_FEATURE_TDX_GUEST, NULL),
> +	{}
> +};
> +MODULE_DEVICE_TABLE(x86cpu, tdx_guest_ids);
> +#endif

Why the #ifdef?  Should not be needed, right?

> +
> +MODULE_AUTHOR("Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>");
> +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..fd71774a90ac
> --- /dev/null
> +++ b/include/uapi/linux/tdx-guest.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_LINUX_TDX_GUEST_H_
> +#define _UAPI_LINUX_TDX_GUEST_H_

No Intel copyright notice?  Are you sure you ran this code through
internal Intel review properly?

{sigh}


> +
> +#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
> +
> +/**
> + * struct tdx_report_req - Get TDREPORT using REPORTDATA as input.
> + *
> + * @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].
> + * @rpd_len: Length of the REPORTDATA (fixed as 64 bytes by the TDX Module
> + *           specification, but a parameter is added to handle future
> + *           extension).
> + * @tdr_len: Length of the TDREPORT (fixed as 1024 bytes by the TDX Module
> + *           specification, but a parameter is added to accommodate future
> + *           extension).
> + * @subtype: Subtype of TDREPORT (fixed as 0 by the TDX Module specification,
> + *           but added a parameter to handle future extension).
> + * @reserved: Reserved entries to handle future requirements. Should be

s/Should/Must/

thanks,

greg k-h