| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c771664d-893b-a41e-079f-74673517b3bb@linux.intel.com>
Date: Thu, 28 Apr 2022 10:56:26 -0700
From: Sathyanarayanan Kuppuswamy
<sathyanarayanan.kuppuswamy@...ux.intel.com>
To: Wander Lairson Costa <wander@...hat.com>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
"H . Peter Anvin" <hpa@...or.com>,
"Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
Tony Luck <tony.luck@...el.com>,
Andi Kleen <ak@...ux.intel.com>,
Kai Huang <kai.huang@...el.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface
driver
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
Powered by blists - more mailing lists