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

[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