Re: [PATCH v3 3/4] virt: tdx-guest: Use a variable to store the Quote buffer size

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] `wait_for_quote_completion` timeout logic is inverted due to an off-by-one error in the loop condition.
--

--- Patch [3]: [PATCH v3 3/4] virt: tdx-guest: Use a variable to store the Quote buffer size ---
commit 0a36558aa4f35238ff748cfc6efbe53e1e0e02ea
Author: Peter Fang <peter.fang@intel.com>

virt: tdx-guest: Use a variable to store the Quote buffer size

This commit replaces the fixed GET_QUOTE_BUF_SIZE constant with a dynamic 
variable to prepare for future dynamic Quote buffer sizes. All users of 
the buffer size have been updated to read from this new variable.

> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
> index f47c5429d002e..3d3f79ab45af0 100644
> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
[ ... ]
> @@ -285,7 +284,7 @@ static int tdx_report_new_locked(struct tsm_report *report, void *data)
>  	if (desc->inblob_len != TDX_REPORTDATA_LEN)
>  		return -EINVAL;
>  
> -	memset(quote_data, 0, GET_QUOTE_BUF_SIZE);
> +	memset(quote_data, 0, quote_data_len);
>  
>  	/* Update Quote buffer header */
>  	quote_buf->version = GET_QUOTE_CMD_VER;
> @@ -296,7 +295,7 @@ static int tdx_report_new_locked(struct tsm_report *report, void *data)
>  	if (ret)
>  		return ret;
>  
> -	err = tdx_hcall_get_quote(quote_data, GET_QUOTE_BUF_SIZE);
> +	err = tdx_hcall_get_quote(quote_data, quote_data_len);
>  	if (err) {
>  		pr_err("GetQuote hypercall failed, status:%llx\n", err);
>  		return -EIO;
>  	}

[Severity: High]
This is a pre-existing issue, but while reviewing this code I noticed a 
logic flaw in wait_for_quote_completion() which is called immediately after 
this hypercall.

drivers/virt/coco/tdx-guest/tdx-guest.c:wait_for_quote_completion() {
    ...
    while (quote_buf->status == GET_QUOTE_IN_FLIGHT && i++ < timeout) {
        if (msleep_interruptible(MSEC_PER_SEC))
            return -EINTR;
    }

    return (i == timeout) ? -ETIMEDOUT : 0;
}

Does this timeout condition return the correct value?

Due to the post-increment in i++ < timeout, if the loop actually times out,
i becomes timeout + 1. This causes i == timeout to evaluate to false and
incorrectly return 0 (success).

Conversely, if the quote succeeds exactly on the last iteration,
short-circuiting leaves i == timeout true, which would incorrectly return
-ETIMEDOUT and cause a valid quote to be rejected.

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