Re: [PATCH v6 5/5] x86/tdx: Add Quote generation support

From: Sathyanarayanan Kuppuswamy
Date: Tue May 17 2022 - 19:32:09 EST



On 5/17/22 4:06 PM, Kai Huang wrote:
On Tue, 2022-05-17 at 13:08 -0700, Sathyanarayanan Kuppuswamy wrote:

On 5/16/22 7:58 PM, Kai Huang wrote:
On Fri, 2022-05-13 at 12:29 -0700, Sathyanarayanan Kuppuswamy wrote:


+
+ /* Wait for attestation completion */
+ ret = wait_for_completion_interruptible(&entry->compl);
+ if (ret < 0) {
+ /*
+ * For premature termination, since VMM still owns the
+ * shared buffer, mark the request invalid to let
+ * quote_callback_handler() handle the memory cleanup
+ * function.
+ */
+ invalidate_quote_request(entry);

Interrupt can arrive after signal interrupt.  So invalidate_quote_request()
should check if the request is already processed, and return 0 or -EINTR.
Probably check the state always and del_list under single lock/unlock pair.

Agree. But I think we should return -EINTR for the interrupted case
irrespective of the processed status (so no return 0).

I will hold the lock and handle the cleanup for the processed
status.

Even if we check the buffer status in invalidate_quote_request(), there's no
guarantee the VMM won't change the buffer status _after_ we do the check, so
looks such check isn't necessary.


Consider the case where we get a callback interrupt, and before we
complete the processing for it, user terminates the request. In this
scenario, quote_callback_handler() will consider the request is
still valid and no do the memory cleanup. To handle this case,
we need to check the status in invalidate_quote_request() and do
the cleanup if required.

/* Handles early termination of GetQuote requests */
void invalidate_quote_request(struct quote_entry *entry)
{
struct tdx_quote_hdr *quote_hdr;

/*
* For early termination, if the request is not yet
* processed by VMM (GET_QUOTE_IN_FLIGHT), the VMM
* still owns the shared buffer, so mark the request
* invalid to let quote_callback_handler() handle the
* memory cleanup function. If the request is already
* processed, then do the cleanup and return.
*/

mutex_lock(&quote_lock);
quote_hdr = (struct tdx_quote_hdr *)entry->buf->vmaddr;
if (quote_hdr->status == GET_QUOTE_IN_FLIGHT) {

What prevents VMM from updating quote_hdr->status from IN_FLIGHT to DONE _after_
this check?

If IN_FLIGHT -> DONE happens at this point, you will get a separate
callback interrupt for it after this change. Since we hold quote_lock
here, processing of that callback work will wait till we make the
request invalid. For invalid requests, quote_callback_handler() will
handle the cleanup.

This logic is mainly for the case where quote_callback_handler() in
process and user terminates the request. In this case,
quote_callback_handler() will not do the cleanup, and we should add
logic here to handle it.


If you want to add such check, you should check against GET_QUOTE_DONE, but not
IN_FLIGHT. Only after status is DONE, VMM will not update the buffer.  Perhaps
something like below:

mutex_lock(&quote_lock);
/* Skip invalidate the buffer if VMM has done with the buffer */
if (quote_hdr->status == GET_QUOTE_DONE) {
mutex_unlock(&quote_lock);
return 0;
}

There is no DONE status. Status can be either one of the following values.

GET_QUOTE_ERROR
GET_QUOTE_SUCCESS
GET_QUOTE_SERVICE_UNAVAILABLE
GET_QUOTE_IN_FLIGHT


And in the IOCTL, you can perhaps to choose to return 0, instead of -EINTR in
this case, as the Quote has been finished already.

I think your logic above is to return 0 for the completed request even
if it is interrupted. But IMO, since the user already interrupted the
request, then I think it is more appropriate to just return -EINTR for
this case.


But I am not sure whether this is necessary. The worst case is one finished
Quote is wasted I guess.


entry->valid = false;
mutex_unlock(&quote_lock);
return;
}
_del_quote_entry(entry);
mutex_unlock(&quote_lock);
}




--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer