Messages in this thread | | | Date | Thu, 28 Apr 2022 10:56:26 -0700 | Subject | Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver | From | Sathyanarayanan Kuppuswamy <> |
| |
Hi,
On 4/28/22 10:45 AM, Wander Lairson Costa wrote: > 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)
Currently, it is a constant value < PAGE_SIZE. But I can add the BUILD_BUG_ON check for it.
> >> + 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
Yes, noted it already. I will remove it in next version.
> >> + 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?
Yes. I have noted it already, I will fix this in next version.
> >> + 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
Agree. It came along with patch split I did. I will remove it in next version.
> >> + } >> + >> + 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.
Yes. It is not required. I will remove it.
> >> + >> + 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?
platform_device_register_simple() seem to trigger probe before it returns. So assigning it in probe is correct. Here it is redundant ( but not harmful)
Anyway this change will go way in next version when I change the driver to be a pure "misc driver" and remove the "platform driver" support.
> >> + 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; >> + } >> + >
-- Sathyanarayanan Kuppuswamy Linux Kernel Developer
| |