linux-kernel - Re: [PATCH 09/21] KVM: TDX: Retry seamcall when TDX_OPERAND

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zt-k30MHhMTxpFKQ@google.com>
Date: Mon, 9 Sep 2024 18:46:07 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Rick P Edgecombe <rick.p.edgecombe@...el.com>
Cc: "pbonzini@...hat.com" <pbonzini@...hat.com>, Yuan Yao <yuan.yao@...el.com>, 
	Kai Huang <kai.huang@...el.com>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Yan Y Zhao <yan.y.zhao@...el.com>, 
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>, "dmatlack@...gle.com" <dmatlack@...gle.com>, 
	"nik.borisov@...e.com" <nik.borisov@...e.com>, 
	"isaku.yamahata@...il.com" <isaku.yamahata@...il.com>
Subject: Re: [PATCH 09/21] KVM: TDX: Retry seamcall when TDX_OPERAND_BUSY with
 operand SEPT

On Tue, Sep 10, 2024, Rick P Edgecombe wrote:
> On Mon, 2024-09-09 at 16:58 -0700, Sean Christopherson wrote:
> > On Mon, Sep 09, 2024, Rick P Edgecombe wrote:
> > > On Mon, 2024-09-09 at 14:23 -0700, Sean Christopherson wrote:
> > > > > 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.
> > > 
> > > Thanks for the analysis. The direction seems reasonable to me for this lock
> > > in
> > > particular. We need to do some analysis on how much the existing mmu_lock
> > > can
> > > protects us. 
> > 
> > I would operate under the assumption that it provides SEPT no meaningful
> > protection.
> > I think I would even go so far as to say that it is a _requirement_ that
> > mmu_lock
> > does NOT provide the ordering required by SEPT, because I do not want to take
> > on
> > any risk (due to SEPT constraints) that would limit KVM's ability to do things
> > while holding mmu_lock for read.
> 
> Ok. Not sure, but I think you are saying not to add any extra acquisitions of
> mmu_lock.

No new write_lock.  If read_lock is truly needed, no worries.  But SEPT needing
a write_lock is likely a hard "no", as the TDP MMU's locking model depends
heavily on vCPUs being readers.  E.g. the TDP MMU has _much_ coarser granularity
than core MM, but it works because almost everything is done while holding
mmu_lock for read.

> Until we answer some of the questions (i.e. HOST_PRIORITY exposure), it's hard
> to say. We need to check some stuff on our end.

Ya, agreed.