Re: [PATCH 09/21] KVM: TDX: Retry seamcall when TDX_OPERAND_BUSY with operand SEPT

[Sean Christopherson <seanjc@google.com>]

On Mon, Sep 09, 2024, Sean Christopherson wrote:
> On Mon, Sep 09, 2024, Rick P Edgecombe wrote:
> > On Mon, 2024-09-09 at 17:25 +0200, Paolo Bonzini wrote:
> > > On 9/4/24 05:07, Rick Edgecombe wrote:
> > > > +static inline u64 tdx_seamcall_sept(u64 op, struct tdx_module_args *in)
> > > > +{
> > > > +#define SEAMCALL_RETRY_MAX     16
> > > 
> > > How is the 16 determined?  Also, is the lock per-VM or global?
> > 
> > The lock being considered here is per-TD, but TDX_OPERAND_BUSY in general can be
> > for other locks. I'm not sure where the 16 came from, maybe Yuan or Isaku can
> > share the history. In any case, there seems to be some problems with this patch
> > or justification.
> > 
> > Regarding the zero-step mitigation, the TDX Module has a mitigation for an
> > attack where a malicious VMM causes repeated private EPT violations for the same
> > GPA. When this happens TDH.VP.ENTER will fail to enter the guest. Regardless of
> > zero-step detection, these SEPT related SEAMCALLs will exit with the checked
> > error code if they contend the mentioned lock. If there was some other (non-
> > zero-step related) contention for this lock and KVM tries to re-enter the TD too
> > many times without resolving an EPT violation, it might inadvertently trigger
> > the zero-step mitigation. I *think* this patch is trying to say not to worry
> > about this case, and do a simple retry loop instead to handle the contention.
> > 
> > But why 16 retries would be sufficient, I can't find a reason for. Getting this
> > required retry logic right is important because some failures
> > (TDH.MEM.RANGE.BLOCK) can lead to KVM_BUG_ON()s.
> 
> I (somewhat indirectly) raised this as an issue in v11, and at a (very quick)
> glance, nothing has changed to alleviate my concerns.

Gah, went out of my way to find the thread and then forgot to post the link:

https://lore.kernel.org/all/Y8m34OEVBfL7Q4Ns@google.com

> In general, I am _very_ opposed to blindly retrying an SEPT SEAMCALL, ever.  For
> its operations, I'm pretty sure the only sane approach is for KVM to ensure there
> will be no contention.  And if the TDX module's single-step protection spuriously
> kicks in, KVM exits to userspace.  If the TDX module can't/doesn't/won't communicate
> that it's mitigating single-step, e.g. so that KVM can forward the information
> to userspace, then that's a TDX module problem to solve.
> 
> > Per the docs, in general the VMM is supposed to retry SEAMCALLs that return
> > TDX_OPERAND_BUSY.
> 
> IMO, that's terrible advice.  SGX has similar behavior, where the xucode "module"
> signals #GP if there's a conflict.  #GP is obviously far, far worse as it lacks
> the precision that would help software understand exactly what went wrong, but I
> think one of the better decisions we made with the SGX driver was to have a
> "zero tolerance" policy where the driver would _never_ retry due to a potential
> resource conflict, i.e. that any conflict in the module would be treated as a
> kernel bug.
> 
> > I think we need to revisit the general question of which
> > SEAMCALLs we should be retrying and how many times/how long. The other
> > consideration is that KVM already has per-VM locking, that would prevent
> > contention for some of the locks. So depending on internal details KVM may not
> > need to do any retries in some cases.
> 
> Yes, and if KVM can't avoid conflict/retry, then before we go any further, I want
> to know exactly why that is the case.