Re: [PATCH v2 2/2] virt: tdx-guest: Allocate Quote buffer dynamically

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Premature exposure of the misc device allows a Use-After-Free (UAF) on the module initialization error path if `alloc_quote_buf()` fails.
--

--- Patch [2]: [PATCH v2 2/2] virt: tdx-guest: Allocate Quote buffer dynamically ---
commit e37e54ca6bcf9f24fca7285847baa3251ff5dfba
Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

virt: tdx-guest: Allocate Quote buffer dynamically

This modifies the TDX attestation driver to dynamically allocate the Quote
buffer based on the size reported by the TDX module. It uses alloc_pages_exact()
to allocate up to a 4MB contiguous buffer and fails quietly if the allocation
cannot be satisfied.

> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
> index d0303e31e8161..ce2c2ef746762 100644
> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c

[ ... ]

> @@ -417,7 +442,7 @@ static int __init tdx_guest_init(void)
>  	if (ret)
>  		goto deinit_mr;
>  
> -	quote_data = alloc_quote_buf();
> +	quote_data = alloc_quote_buf(&quote_data_len);

[Severity: High]
This is a pre-existing issue, but this patch makes it significantly more
likely to trigger by introducing a large contiguous memory allocation (up to
4MB) that can easily fail on fragmented systems.

Can this lead to a Use-After-Free on the initialization error path?

If misc_register(&tdx_misc_dev) (called just before this hunk) succeeds, 
the /dev/tdx_guest device is immediately exposed to userspace. A userspace 
process can open it and call ioctl(TDX_CMD_GET_REPORT0), entering
tdx_do_report() which uses tdx_report_buf.

If alloc_quote_buf() then fails, the error path calls misc_deregister() and 
tdx_mr_deinit(). 
    
tdx_mr_deinit() executes:

    kfree(tdx_report_buf);
    
This freeing happens without acquiring mr_lock, meaning it can race with the 
ongoing ioctl. Could the concurrent ioctl in tdx_do_report() then access the 
freed tdx_report_buf?

Should misc_register() be moved to the end of tdx_guest_init() after all
resource allocations succeed?

>  	if (!quote_data) {
>  		pr_err("Failed to allocate Quote buffer\n");
>  		ret = -ENOMEM;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717214349.4075994-1-peter.fang@intel.com?part=2