Re: [PATCH v7 7/7] virt: tdx-guest: Add Quote generation support using TSM_REPORTS

[Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>]

On 12/20/2023 5:50 PM, Xiaoyao Li wrote:
> On 10/20/2023 9:17 AM, Dan Williams wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ...
>> +static int tdx_report_new(struct tsm_report *report, void *data)
>> +{
>> +    u8 *buf, *reportdata = NULL, *tdreport = NULL;
>> +    struct tdx_quote_buf *quote_buf = quote_data;
>> +    struct tsm_desc *desc = &report->desc;
>> +    int ret;
>> +    u64 err;
>> +
>> +    /* TODO: switch to guard(mutex_intr) */
>> +    if (mutex_lock_interruptible(&quote_lock))
>> +        return -EINTR;
>> +
>> +    /*
>> +     * If the previous request is timedout or interrupted, and the
>> +     * Quote buf status is still in GET_QUOTE_IN_FLIGHT (owned by
>> +     * VMM), don't permit any new request.
>> +     */
>> +    if (quote_buf->status == GET_QUOTE_IN_FLIGHT) {
>> +        ret = -EBUSY;
>> +        goto done;
>> +    }
>> +
>> +    if (desc->inblob_len != TDX_REPORTDATA_LEN) {
>> +        ret = -EINVAL;
>> +        goto done;
>> +    }
>> +
>> +    reportdata = kmalloc(TDX_REPORTDATA_LEN, GFP_KERNEL);
>> +    if (!reportdata) {
>> +        ret = -ENOMEM;
>> +        goto done;
>> +    }
>> +
>> +    tdreport = kzalloc(TDX_REPORT_LEN, GFP_KERNEL);
>> +    if (!tdreport) {
>> +        ret = -ENOMEM;
>> +        goto done;
>> +    }
>> +
>> +    memcpy(reportdata, desc->inblob, desc->inblob_len);
>> +
>> +    /* Generate TDREPORT0 using "TDG.MR.REPORT" TDCALL */
>> +    ret = tdx_mcall_get_report0(reportdata, tdreport);
>> +    if (ret) {
>> +        pr_err("GetReport call failed\n");
>> +        goto done;
>> +    }
>> +
>> +    memset(quote_data, 0, GET_QUOTE_BUF_SIZE);
>> +
>> +    /* Update Quote buffer header */
>> +    quote_buf->version = GET_QUOTE_CMD_VER;
>> +    quote_buf->in_len = TDX_REPORT_LEN;
>> +
>> +    memcpy(quote_buf->data, tdreport, TDX_REPORT_LEN);
>> +
>> +    err = tdx_hcall_get_quote(quote_data, GET_QUOTE_BUF_SIZE);
>> +    if (err) {
>> +        pr_err("GetQuote hypercall failed, status:%llx\n", err);
>> +        ret = -EIO;
>> +        goto done;
>> +    }
>> +
>> +    ret = wait_for_quote_completion(quote_buf, getquote_timeout);
>> +    if (ret) {
>> +        pr_err("GetQuote request timedout\n");
>> +        goto done;
>> +    }
> 
> Sorry that I didn't check the previous discussion and don't know if it is by design or not:
> 
> Why don't check the quote_buf->status? If it indicates errors, we should return some error code instead, right?

For the failed request, outblob_len will be zero and the empty output can be
treated as failed request. But I agree that it makes sense to return error
for the failed request. I can submit a patch for it. Something like below:

--- a/drivers/virt/coco/tdx-guest/tdx-guest.c
+++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
@@ -33,6 +33,8 @@

 /* TDX GetQuote status codes */
 #define GET_QUOTE_SUCCESS              0
+#define GET_QUOTE_ERROR                        0x8000000000000000
+#define GET_QUOTE_SERVICE_UNAVAILABLE  0x8000000000000001
 #define GET_QUOTE_IN_FLIGHT            0xffffffffffffffff

 /* struct tdx_quote_buf: Format of Quote request buffer.
@@ -228,6 +230,12 @@ static int tdx_report_new(struct tsm_report *report, void *data)
                goto done;
        }

+       if (quote_buf->status != GET_QUOTE_SUCCESS) {
+               pr_err("GetQuote request failed, ret %llx\n", quote_buf->status);
+               ret = -EIO;
+               goto done;
+       }
+


> 
>> +    buf = kvmemdup(quote_buf->data, quote_buf->out_len, GFP_KERNEL);
>> +    if (!buf) {
>> +        ret = -ENOMEM;
>> +        goto done;
>> +    }
>> +
>> +    report->outblob = buf;
>> +    report->outblob_len = quote_buf->out_len;
>> +
>> +    /*
>> +     * TODO: parse the PEM-formatted cert chain out of the quote buffer when
>> +     * provided
>> +     */
>> +done:
>> +    mutex_unlock(&quote_lock);
>> +    kfree(reportdata);
>> +    kfree(tdreport);
>> +
>> +    return ret;
>> +}
>> +
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer