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

[Kuppuswamy, Sathyanarayanan]

On 7/20/21 9:53 AM, Dave Hansen wrote:
> > +/* 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

I can change it to 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

From this driver perspective, they are opaque blobs. We don't have any
need to access it. Once this data is passed back to user-agent,
it can decode it appropriately. The data format of this blob is defined
by TDX Module spec.

> actual data type?

If void * is not good, may be we can use u8*. But we really don't access it.

> > +/* 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.

Ok.

> > +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?

Both are same. But, if this makes it easier to understand, I can move the
virt_to_phys() inside the tdx_mcall_tdreport() helper function.

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

Ok. I can rename them as tdreport_data -> tdreport_output and report_data ->
report_input.

> > +			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?

Good catch. I need to set ret=-EIO here. I will also remove the pr_err.

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

It doesn't need to be long. I will change it to int.

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

I have zeroed out the memory to make it easier to test whether
TDX_CMD_GET_TDREPORT module call works or not. Not a TDX module requirement.

I will remove _GFP_ZERO flag in next version.

> only needs a single page, why not use __get_free_page()?

Yes, I only need one page. I will use __get_free_page().

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer