Re: [PATCH v3 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Dave Hansen <dave.hansen@intel.com>]

> +/* Used in Quote memory allocation */
> +#define QUOTE_SIZE			(2 * PAGE_SIZE)
> +/* Get Quote timeout in msec */
> +#define GET_QUOTE_TIMEOUT		(5000)

The comment is good, but even better would be to call this:

	GET_QUOTE_TIMEOUT_MS

> +/* Mutex to synchronize attestation requests */
> +static DEFINE_MUTEX(attestation_lock);
> +/* Completion object to track attestation status */
> +static DECLARE_COMPLETION(attestation_done);
> +/* Buffer used to copy report data in attestation handler */
> +static u8 report_data[TDX_REPORT_DATA_LEN];
> +/* Data pointer used to get TD Quote data in attestation handler */
> +static void *tdquote_data;
> +/* Data pointer used to get TDREPORT data in attestation handler */
> +static void *tdreport_data;

Are these *really* totally unknown, opaque blobs?  Why not give them an
actual data type?

> +/* DMA handle used to allocate and free tdquote DMA buffer */
> +dma_addr_t tdquote_dma_handle;

That's an unreadable jumble.  Please add some line breaks and try to
logically group those.

> +static void attestation_callback_handler(void)
> +{
> +	complete(&attestation_done);
> +}
> +
> +static long tdg_attest_ioctl(struct file *file, unsigned int cmd,
> +			     unsigned long arg)
> +{
> +	void __user *argp = (void __user *)arg;
> +	long ret = 0;
> +
> +	mutex_lock(&attestation_lock);
> +
> +	switch (cmd) {
> +	case TDX_CMD_GET_TDREPORT:
> +		if (copy_from_user(report_data, argp, TDX_REPORT_DATA_LEN)) {
> +			ret = -EFAULT;
> +			break;
> +		}
> +
> +		/* Generate TDREPORT_STRUCT */
> +		if (tdx_mcall_tdreport(virt_to_phys(tdreport_data),
> +				       virt_to_phys(report_data))) {

Having that take a physical address seems like a mistake.  Why not just
do the virt_to_phys() inside the helper?

Also, this isn't very clear that there is an input and an output.  Can
you rename these to make that more clear?

> +			ret = -EIO;
> +			break;
> +		}
> +
> +		if (copy_to_user(argp, tdreport_data, TDX_TDREPORT_LEN))
> +			ret = -EFAULT;
> +		break;
> +	case TDX_CMD_GEN_QUOTE:
> +		/* Copy TDREPORT data from user buffer */
> +		if (copy_from_user(tdquote_data, argp, TDX_TDREPORT_LEN)) {
> +			ret = -EFAULT;
> +			break;
> +		}
> +
> +		/* Submit GetQuote Request */
> +		if (tdx_hcall_get_quote(virt_to_phys(tdquote_data))) {
> +			ret = -EIO;
> +			break;
> +		}
> +
> +		/* Wait for attestation completion */
> +		ret = wait_for_completion_interruptible_timeout(
> +				&attestation_done,
> +				msecs_to_jiffies(GET_QUOTE_TIMEOUT));
> +		if (ret <= 0) {
> +			ret = -EIO;
> +			break;
> +		}
> +
> +		if (copy_to_user(argp, tdquote_data, QUOTE_SIZE))
> +			ret = -EFAULT;
> +
> +		break;
> +	case TDX_CMD_GET_QUOTE_SIZE:
> +		ret = put_user(QUOTE_SIZE, (u64 __user *)argp);
> +		break;
> +	default:
> +		pr_err("cmd %d not supported\n", cmd);
> +		break;

First of all, drivers shouldn't pollute the kernel log on bad input.
Second, won't this inherit the ret=0 value and return success?

> +	}
> +
> +	mutex_unlock(&attestation_lock);
> +
> +	return ret;
> +}
> +
> +static const struct file_operations tdg_attest_fops = {
> +	.owner		= THIS_MODULE,
> +	.unlocked_ioctl	= tdg_attest_ioctl,
> +	.llseek		= no_llseek,
> +};
> +
> +static struct miscdevice tdg_attest_device = {
> +	.minor          = MISC_DYNAMIC_MINOR,
> +	.name           = "tdx-attest",
> +	.fops           = &tdg_attest_fops,
> +};
> +
> +static int __init tdg_attest_init(void)
> +{
> +	dma_addr_t handle;
> +	long ret = 0;

The function returns 'int', yet 'ret' is a long.  Why?

> +	ret = misc_register(&tdg_attest_device);
> +	if (ret) {
> +		pr_err("misc device registration failed\n");
> +		return ret;
> +	}
> +
> +	tdreport_data = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 0);
> +	if (!tdreport_data) {
> +		ret = -ENOMEM;
> +		goto failed;
> +	}

Why does this need to use the page allocator directly?  Why does it need
to zero the memory?  Why does it need to get a whole page?  If it really
only needs a single page, why not use __get_free_page()?

> +
> +	ret = dma_set_coherent_mask(tdg_attest_device.this_device,
> +				    DMA_BIT_MASK(64));
> +	if (ret) {
> +		pr_err("dma set coherent mask failed\n");
> +		goto failed;
> +	}
> +
> +	/* Allocate DMA buffer to get TDQUOTE data from the VMM */
> +	tdquote_data = dma_alloc_coherent(tdg_attest_device.this_device,
> +					  QUOTE_SIZE, &handle,
> +					  GFP_KERNEL | __GFP_ZERO);
> +	if (!tdquote_data) {
> +		ret = -ENOMEM;
> +		goto failed;
> +	}
> +
> +	tdquote_dma_handle =  handle;
> +
> +	/*
> +	 * Currently tdg_event_notify_handler is only used in attestation
> +	 * driver. But, WRITE_ONCE is used as benign data race notice.
> +	 */
> +	WRITE_ONCE(tdg_event_notify_handler, attestation_callback_handler);
> +
> +	pr_debug("module initialization success\n");
> +
> +	return 0;
> +
> +failed:
> +	if (tdreport_data)
> +		free_pages((unsigned long)tdreport_data, 0);
> +
> +	misc_deregister(&tdg_attest_device);
> +
> +	pr_debug("module initialization failed\n");
> +
> +	return ret;
> +}
...