Re: [PATCH v5 3/3] x86/tdx: Add Quote generation support

[Kai Huang]

On Thu, 2022-05-05 at 12:03 -0700, Sathyanarayanan Kuppuswamy wrote:
> Hi Kai,
> 
> On 5/5/22 3:50 AM, Kai Huang wrote:
> > 
> > > +       /* Submit GetQuote Request */
> > > +       ret = tdx_get_quote_hypercall(buf);
> > > +       if (ret) {
> > > +               pr_err("GetQuote hypercall failed, status:%lx\n", ret);
> > > +               ret = -EIO;
> > > +               goto free_entry;
> > > +       }
> > > +
> > > +       /* Add current quote entry to quote_list */
> > > +       add_quote_entry(entry);
> > > +
> > > +       /* Wait for attestation completion */
> > > +       ret = wait_for_completion_interruptible(&entry->compl);
> > > +       if (ret < 0) {
> > > +               ret = -EIO;
> > > +               goto del_entry;
> > > +       }
> > 
> > This is misuse of wait_for_completion_interruptible().
> > 
> > xxx_interruptible() essentially means this operation can be interrupted by
> > signal.  Using xxx_interruptible() in driver IOCTL essentially means when it
> > returns due to signal, the IOCTL should return -EINTR to let userspace know that
> > your application received some signal needs handling, and this IOCTL isn't
> > finished and you should retry.  So here we should return -EINTR (and cleanup all
> > staff has been done) when wait_for_completion_interruptible() returns -
> > ERESTARTSYS (in fact, it returns only -ERESTARTSYS or 0).
> 
> 
> But in this case, I was expecting the user agent to check the Quote
> buffer status code to understand the IN_FLIGHT, SUCCESS or FAILURE
> status and handle it appropriately. So IMO, it should not matter what
> we return for the failure case. For the IN_FLIGHT case, they can retry
> if they want after checking the status code.

Couple of issues around your statement:

1) When wait_for_completion_interruptible() returns error, you never copied the
data back to userspace.  Therefore userspace cannot check buffer status.  Your
assumption is wrong.
2) Even if you copy the data back to userspace, there's no guarantee *after* you
do the copy, the VMM won't update the buffer status.  So this doesn't work.
3) You only provide one IOCTL.  Even if userspace can retry, you will create a
new buffer, and ask VMM to do it again.  Then what happens to the old buffer? 
VMM is still owning it, and can update it, but you have already converted it
back to private, and freed it.

I really don't see how your statement is correct.

> 
> But I agree that EINTR is the appropriate return value for an
> interrupted case. So, I will change it.
> 
> > 
> > Since normally userspace application just ignore signals, and in this particular
> > case, asking userspace to retry just makes things more complicated to handle, I
> 
> I am not sure how the user agent is going to be implemented. So I don't
> want to make any assumptions. In this case, we are not asking user space
> to implement the retry support using signals. But we are just giving 
> them option to do it. It is up to them if they want to use it.

As I see you are not providing any functionality to allow userspace to retry.



-- 
Thanks,
-Kai