| Message ID | 20220908002723.923241-2-sathyanarayanan.kuppuswamy@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add TDX Guest Attestation support | expand |
On Wed, Sep 07, 2022 at 05:27:20PM -0700, Kuppuswamy Sathyanarayanan wrote: > + /* > + * 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. Also check for valid user pointers. > + */ > + if (!req.reportdata || !req.tdreport || req.subtype || > + req.rpd_len != TDX_REPORTDATA_LEN || > + req.tdr_len != TDX_REPORT_LEN) > + return -EINVAL; You never verify that your reserved[7] fields are actually set to 0, which means you can never use them in the future :( Please fix that up, thanks. greg k-h
Hi, On 9/7/22 10:31 PM, Greg Kroah-Hartman wrote: > On Wed, Sep 07, 2022 at 05:27:20PM -0700, Kuppuswamy Sathyanarayanan wrote: >> + /* >> + * 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. Also check for valid user pointers. >> + */ >> + if (!req.reportdata || !req.tdreport || req.subtype || >> + req.rpd_len != TDX_REPORTDATA_LEN || >> + req.tdr_len != TDX_REPORT_LEN) >> + return -EINVAL; > > You never verify that your reserved[7] fields are actually set to 0, > which means you can never use them in the future :( Currently, we don't use those fields in our code. Why do we have to make sure they are set to zero? Can't we add checks when we really use them in future? If your suggestion is to define allowed values of these fields for user, we can add some help in structure definition of "tdx_report_req" in arch/x86/include/uapi/asm/tdx.h > > Please fix that up, thanks. > > greg k-h
On 9/8/22 12:07, Sathyanarayanan Kuppuswamy wrote: > On 9/7/22 10:31 PM, Greg Kroah-Hartman wrote: >> On Wed, Sep 07, 2022 at 05:27:20PM -0700, Kuppuswamy Sathyanarayanan wrote: >>> + /* >>> + * 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. Also check for valid user pointers. >>> + */ >>> + if (!req.reportdata || !req.tdreport || req.subtype || >>> + req.rpd_len != TDX_REPORTDATA_LEN || >>> + req.tdr_len != TDX_REPORT_LEN) >>> + return -EINVAL; >> You never verify that your reserved[7] fields are actually set to 0, >> which means you can never use them in the future :( > Currently, we don't use those fields in our code. Why do we have to > make sure they are set to zero? Yes. > Can't we add checks when we really use them in future? No. This has been a hard learned lesson both by people writing software and designing hardware interfaces: if you _let_ folks pass garbage you have to _keep_ letting them pass garbage forever. It becomes part of the ABI. I'm sorry you missed the memo on this one. But, this is one million percent a best practice across the industry. Please do it.
On 9/8/22 1:36 PM, Dave Hansen wrote: > On 9/8/22 12:07, Sathyanarayanan Kuppuswamy wrote: >> On 9/7/22 10:31 PM, Greg Kroah-Hartman wrote: >>> On Wed, Sep 07, 2022 at 05:27:20PM -0700, Kuppuswamy Sathyanarayanan wrote: >>>> + /* >>>> + * 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. Also check for valid user pointers. >>>> + */ >>>> + if (!req.reportdata || !req.tdreport || req.subtype || >>>> + req.rpd_len != TDX_REPORTDATA_LEN || >>>> + req.tdr_len != TDX_REPORT_LEN) >>>> + return -EINVAL; >>> You never verify that your reserved[7] fields are actually set to 0, >>> which means you can never use them in the future :( >> Currently, we don't use those fields in our code. Why do we have to >> make sure they are set to zero? > > Yes. > >> Can't we add checks when we really use them in future? > > No. > > This has been a hard learned lesson both by people writing software and > designing hardware interfaces: if you _let_ folks pass garbage you have > to _keep_ letting them pass garbage forever. It becomes part of the ABI. > > I'm sorry you missed the memo on this one. But, this is one million > percent a best practice across the industry. Please do it. Ok. Thanks for clarifying it. I will fix it in next version.
Hi Dave/Greg, On 9/7/22 10:31 PM, Greg Kroah-Hartman wrote: > On Wed, Sep 07, 2022 at 05:27:20PM -0700, Kuppuswamy Sathyanarayanan wrote: >> + /* >> + * 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. Also check for valid user pointers. >> + */ >> + if (!req.reportdata || !req.tdreport || req.subtype || >> + req.rpd_len != TDX_REPORTDATA_LEN || >> + req.tdr_len != TDX_REPORT_LEN) >> + return -EINVAL; > > You never verify that your reserved[7] fields are actually set to 0, > which means you can never use them in the future :( > > Please fix that up, thanks. Would you prefer a new posting (v12.1 or v13) with this change, or do you want to continue the review in the same version? > > greg k-h
On Thu, Sep 08, 2022 at 04:53:18PM -0700, Sathyanarayanan Kuppuswamy wrote: > Hi Dave/Greg, > > On 9/7/22 10:31 PM, Greg Kroah-Hartman wrote: > > On Wed, Sep 07, 2022 at 05:27:20PM -0700, Kuppuswamy Sathyanarayanan wrote: > >> + /* > >> + * 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. Also check for valid user pointers. > >> + */ > >> + if (!req.reportdata || !req.tdreport || req.subtype || > >> + req.rpd_len != TDX_REPORTDATA_LEN || > >> + req.tdr_len != TDX_REPORT_LEN) > >> + return -EINVAL; > > > > You never verify that your reserved[7] fields are actually set to 0, > > which means you can never use them in the future :( > > > > Please fix that up, thanks. > > Would you prefer a new posting (v12.1 or v13) with this change, or do you > want to continue the review in the same version? What would you want to see happen if you were a reviewer? (hint, new version...)
On Thu, Sep 08, 2022 at 01:36:00PM -0700, Dave Hansen wrote: > On 9/8/22 12:07, Sathyanarayanan Kuppuswamy wrote: > > On 9/7/22 10:31 PM, Greg Kroah-Hartman wrote: > >> On Wed, Sep 07, 2022 at 05:27:20PM -0700, Kuppuswamy Sathyanarayanan wrote: > >>> + /* > >>> + * 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. Also check for valid user pointers. > >>> + */ > >>> + if (!req.reportdata || !req.tdreport || req.subtype || > >>> + req.rpd_len != TDX_REPORTDATA_LEN || > >>> + req.tdr_len != TDX_REPORT_LEN) > >>> + return -EINVAL; > >> You never verify that your reserved[7] fields are actually set to 0, > >> which means you can never use them in the future :( > > Currently, we don't use those fields in our code. Why do we have to > > make sure they are set to zero? > > Yes. > > > Can't we add checks when we really use them in future? > > No. > > This has been a hard learned lesson both by people writing software and > designing hardware interfaces: if you _let_ folks pass garbage you have > to _keep_ letting them pass garbage forever. It becomes part of the ABI. > > I'm sorry you missed the memo on this one. But, this is one million > percent a best practice across the industry. Please do it. And it's documented in the Documentation/ directory as a requirement to do as well, the memo shouldn't have been missed :(
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c index 928dcf7a20d9..d1ea7dae3f20 100644 --- a/arch/x86/coco/tdx/tdx.c +++ b/arch/x86/coco/tdx/tdx.c @@ -5,16 +5,21 @@ #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> #include <asm/insn.h> #include <asm/insn-eval.h> #include <asm/pgtable.h> +#include <uapi/asm/tdx.h> /* TDX module Call Leaf IDs */ #define TDX_GET_INFO 1 #define TDX_GET_VEINFO 3 +#define TDX_GET_REPORT 4 #define TDX_ACCEPT_PAGE 6 /* TDX hypercall Leaf IDs */ @@ -775,3 +780,110 @@ void __init tdx_early_init(void) pr_info("Guest detected\n"); } + +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. Also check for valid user pointers. + */ + if (!req.reportdata || !req.tdreport || req.subtype || + req.rpd_len != TDX_REPORTDATA_LEN || + req.tdr_len != TDX_REPORT_LEN) + return -EINVAL; + + reportdata = kmalloc(req.rpd_len, GFP_KERNEL); + if (!reportdata) + return -ENOMEM; + + tdreport = kzalloc(req.tdr_len, GFP_KERNEL); + if (!tdreport) { + kfree(reportdata); + return -ENOMEM; + } + + 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. + * + * 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, + 0, NULL); + if (ret) { + ret = -EIO; + 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) +{ + void __user *argp = (void __user *)arg; + long ret = -ENOTTY; + + 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 struct miscdevice tdx_misc_dev = { + .name = TDX_GUEST_DEVICE, + .minor = MISC_DYNAMIC_MINOR, + .fops = &tdx_guest_fops, +}; + +static int __init tdx_guest_init(void) +{ + int ret; + + if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) + return -EIO; + + ret = misc_register(&tdx_misc_dev); + if (ret) { + pr_err("misc device registration failed\n"); + return ret; + } + + return 0; +} +device_initcall(tdx_guest_init) diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h new file mode 100644 index 000000000000..29d8e38f226a --- /dev/null +++ b/arch/x86/include/uapi/asm/tdx.h @@ -0,0 +1,54 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +#ifndef _UAPI_ASM_X86_TDX_H +#define _UAPI_ASM_X86_TDX_H + +#include <linux/types.h> +#include <linux/ioctl.h> + +#define TDX_GUEST_DEVICE "tdx-guest" + +/* 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 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 TDX Module + * specification, but added a parameter to handle + * future extension). + * + * Used in TDX_CMD_GET_REPORT IOCTL request. + */ +struct tdx_report_req { + __u64 reportdata; + __u64 tdreport; + __u32 rpd_len; + __u32 tdr_len; + __u8 subtype; + __u8 reserved[7]; +}; + +/* + * TDX_CMD_GET_REPORT - Get TDREPORT using TDCALL[TDG.MR.REPORT] + * + * Return 0 on success, -EIO on TDCALL execution failure, and + * standard errno on other general error cases. + * + */ +#define TDX_CMD_GET_REPORT _IOWR('T', 0x01, __u64) + +#endif /* _UAPI_ASM_X86_TDX_H */