Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver

Wander Lairson Costa
2022-04-28T17:45:24Z

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;
> + }
> +