Hi,
On 10/19/22 10:38 PM, Greg Kroah-Hartman wrote:
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.
Added it to help userspace understand the reason for the failure (only for the cases like request param issues and TDCALL failure). Boris recommended adding it in the previous review.
Also, you have a real device here, use it and call dev_*() instead of pr_*(). Your code should not have any pr_* calls.
Ok. I will use dev_err variant.
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?
I have added it to fix the following warning reported by 0-day.
https://lore.kernel.org/lkml/202209211607.tCtTWKbV-lkp@intel.com/
It is related to nullifying the MODULE_DEVICE_TABLE in #ifndef MODULE case in linux/module.h.
thanks,
greg k-h