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;
+}...