Messages in this thread | | | Date | Thu, 28 Apr 2022 14:45:12 -0300 | From | Wander Lairson Costa <> | Subject | Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver |
| |
On Fri, Apr 22, 2022 at 04:34:16PM -0700, Kuppuswamy Sathyanarayanan wrote:
[snip]
> +static long tdx_get_tdreport(void __user *argp) > +{ > + void *report_buf = NULL, *tdreport_buf = NULL; > + long ret = 0, err; > + > + /* Allocate space for report data */ > + report_buf = kmalloc(TDX_REPORT_DATA_LEN, GFP_KERNEL); > + if (!report_buf) > + return -ENOMEM; > + > + /* > + * Allocate space for TDREPORT buffer (1024-byte aligned). > + * Full page alignment is more than enough. > + */ > + tdreport_buf = (void *)get_zeroed_page(GFP_KERNEL);
Maybe we should add BUILD_BUG_ON(TDX_TDREPORT_LEN > PAGE_SIZE)
> + if (!tdreport_buf) { > + ret = -ENOMEM; > + goto tdreport_failed; > + } > + > + /* Copy report data to kernel buffer */ > + if (copy_from_user(report_buf, argp, TDX_REPORT_DATA_LEN)) { > + ret = -EFAULT; > + goto tdreport_failed; > + } > + > + /* Generate TDREPORT using report data in report_buf */ > + err = tdx_mcall_tdreport(tdreport_buf, report_buf); > + if (err) { > + /* If failed, pass TDCALL error code back to user */ > + ret = put_user(err, (long __user *)argp);
The assigment to ret is useless here
> + ret = -EIO; > + goto tdreport_failed; > + } > + > + /* Copy TDREPORT data back to user buffer */ > + if (copy_to_user(argp, tdreport_buf, TDX_TDREPORT_LEN)) > + ret = -EFAULT; > + > +tdreport_failed: > + kfree(report_buf); > + if (tdreport_buf) > + free_pages((unsigned long)tdreport_buf, 0); > + > + return ret; > +} > + > +static long tdx_attest_ioctl(struct file *file, unsigned int cmd, > + unsigned long arg) > +{ > + void __user *argp = (void __user *)arg; > + long ret = 0; > + > + switch (cmd) { > + case TDX_CMD_GET_TDREPORT: > + ret = tdx_get_tdreport(argp); > + break; > + default: > + pr_err("cmd %d not supported\n", cmd);
Shouldn't we add "ret = -EINVAL" here?
> + break; > + } > + > + return ret; > +} > + > +static const struct file_operations tdx_attest_fops = { > + .owner = THIS_MODULE, > + .unlocked_ioctl = tdx_attest_ioctl, > + .llseek = no_llseek, > +}; > + > +static int tdx_attest_probe(struct platform_device *attest_pdev) > +{ > + struct device *dev = &attest_pdev->dev; > + long ret = 0; > + > + /* Only single device is allowed */ > + if (pdev) > + return -EBUSY; > + > + pdev = attest_pdev; > + > + miscdev.name = DRIVER_NAME; > + miscdev.minor = MISC_DYNAMIC_MINOR; > + miscdev.fops = &tdx_attest_fops; > + miscdev.parent = dev; > + > + ret = misc_register(&miscdev); > + if (ret) { > + pr_err("misc device registration failed\n"); > + goto failed;
Why just not return error here? There is nothing to cleanup
> + } > + > + pr_debug("module initialization success\n"); > + > + return 0; > + > +failed: > + misc_deregister(&miscdev);
The only way to get here is if misc_register fails, so we don't need this call here.
> + > + pr_debug("module initialization failed\n"); > + > + return ret; > +} > + > +static int tdx_attest_remove(struct platform_device *attest_pdev) > +{ > + misc_deregister(&miscdev); > + pr_debug("module is successfully removed\n"); > + return 0; > +} > + > +static struct platform_driver tdx_attest_driver = { > + .probe = tdx_attest_probe, > + .remove = tdx_attest_remove, > + .driver = { > + .name = DRIVER_NAME, > + }, > +}; > + > +static int __init tdx_attest_init(void) > +{ > + int ret; > + > + /* Make sure we are in a valid TDX platform */ > + if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) > + return -EIO; > + > + ret = platform_driver_register(&tdx_attest_driver); > + if (ret) { > + pr_err("failed to register driver, err=%d\n", ret); > + return ret; > + } > + > + pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
pdev is assigned here and in the probe function. Is it correct?
> + if (IS_ERR(pdev)) { > + ret = PTR_ERR(pdev); > + pr_err("failed to allocate device, err=%d\n", ret); > + platform_driver_unregister(&tdx_attest_driver); > + return ret; > + } > +
| |