| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 20 Oct 2022 07:38:40 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@...ux.intel.com>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
Shuah Khan <shuah@...nel.org>,
Jonathan Corbet <corbet@....net>,
"H . Peter Anvin" <hpa@...or.com>,
"Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
Tony Luck <tony.luck@...el.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, linux-kselftest@...r.kernel.org,
linux-doc@...r.kernel.org
Subject: Re: [PATCH v15 2/3] virt: Add TDX guest driver
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@...ux.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
Powered by blists - more mailing lists