Thread (13 messages) 13 messages, 4 authors, 2021-07-28

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

From: Dave Hansen <hidden>
Date: 2021-07-20 16:54:12
Also in: bpf, lkml, platform-driver-x86

+/* 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;
+}
...
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help