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