Messages in this thread | | | Subject | Re: [PATCH v5 3/3] x86/tdx: Add Quote generation support | From | Kai Huang <> | Date | Tue, 03 May 2022 11:02:26 +1200 |
| |
> > > > > Also, the buffer may be still used by VMM when timeout (IN_FLIGHT), how can > > this even work? > > We will never reach here for IN_FLIGHT case. We will block in > wait_for_completion_interruptible() till the status changes to success > or failure.
But you still have 'timeout' in userspace ABI?
> > > > > > + tdquote = NULL; > > > + mutex_unlock("e_lock); > > > + return ret; > > > +} > > > + > > > +static void attestation_callback_handler(void) > > > +{ > > > + struct tdx_quote_hdr *quote_hdr; > > > + > > > + quote_hdr = (struct tdx_quote_hdr *) tdquote; > > > + > > > + /* Check for spurious callback IRQ case */ > > > + if (!tdquote || quote_hdr->status == GET_QUOTE_IN_FLIGHT) > > > + return; > > > > I don't get the logic. Please explain. > > I am trying to handle spurious IRQ case here. If we receive a callback > IRQ from VMM before even we allocate tdquote or post the GetQuote > request, accessing quote_hdr->status will lead to NULL pointer > exception. So I have added check for valid quote buffer (tdquote != > NULL)
tdquote here isn't protected mutex, so even after you check tdquote != NULL, you cannot guarantee it is still valid when you check quote_hdr.
For instance, you receive two interrupts in very short time. The first interrupt gets you out from wait_for_completion_interruptible(). Then second interrupt comes, and you can potentially have situation like below:
cpu 0 cpu 1
tdx_get_quote attestation_callback_hander
!tdquote ... tdquote = NULL;
quote_hdr->status == ...
And the 'quote_hdr' is even initialized at the beginning, and when it is used, the actual tdquote may already been freed.
Or did I get it wrong?
> > Second condition (quote_hdr->status == GET_QUOTE_IN_FLIGHT)) makes > sure we don't mark the current quote request complete until the > Quote buffer status changes to GET_QUOTE_SUCCESS, GET_QUOTE_ERROR or > GET_QUOTE_SERVICE_UNAVAILABLE.
I thought the GHCI spec says VMM will always inject intererupt after the Quote is ready, so we can have assumption it won't inject when buffer is still IN_FLIGHT. But I am also not so sure.
Anyway, to me it seems there's ambiguities around GetQuote and SetupEventNotifyInterrupt. This is the reason that I suggested you to split the basic driver out and you can even upstream it first w/o GetQuote support. I think you may want to actually start to consider to upstream the basic driver first.
-- Thanks, -Kai
| |