Received-SPF: pass (google.com: domain of linux-kernel+bounces-147349-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 16 Apr 2024 11:07:55 -0700
In-Reply-To: <c2ca06e2d8d7ef66800f012953b8ea4be0147c92.camel@cyberus-technology.de>
Precedence: bulk
Mime-Version: 1.0
References: <20240416123558.212040-1-julian.stecklina@cyberus-technology.de>
 <Zh6MmgOqvFPuWzD9@google.com> <ecb314c53c76bc6d2233a8b4d783a15297198ef8.camel@cyberus-technology.de>
 <Zh6WlOB8CS-By3DQ@google.com> <c2ca06e2d8d7ef66800f012953b8ea4be0147c92.camel@cyberus-technology.de>
Message-ID: <Zh6-e9hy7U6DD2QM@google.com>
Subject: Re: [PATCH 1/2] KVM: nVMX: fix CR4_READ_SHADOW when L0 updates CR4
 during a signal
From: Sean Christopherson <seanjc@google.com>
To: Thomas Prescher <thomas.prescher@cyberus-technology.de>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "x86@kernel.org" <x86@kernel.org>, 
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>, "hpa@zytor.com" <hpa@zytor.com>, 
	Julian Stecklina <julian.stecklina@cyberus-technology.de>, 
	"tglx@linutronix.de" <tglx@linutronix.de>, "bp@alien8.de" <bp@alien8.de>, "mingo@redhat.com" <mingo@redhat.com>, 
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>, "pbonzini@redhat.com" <pbonzini@redhat.com>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Apr 16, 2024, Thomas Prescher wrote:
> On Tue, 2024-04-16 at 08:17 -0700, Sean Christopherson wrote:
> > On Tue, Apr 16, 2024, Thomas Prescher wrote:
> > > Hi Sean,
> > >=20
> > > On Tue, 2024-04-16 at 07:35 -0700, Sean Christopherson wrote:
> > > > On Tue, Apr 16, 2024, Julian Stecklina wrote:
> > > > > From: Thomas Prescher <thomas.prescher@cyberus-technology.de>
> > > > >=20
> > > > > This issue occurs when the kernel is interrupted by a signal whil=
e
> > > > > running a L2 guest. If the signal is meant to be delivered to the=
 L0
> > > > > VMM, and L0 updates CR4 for L1, i.e. when the VMM sets
> > > > > KVM_SYNC_X86_SREGS in kvm_run->kvm_dirty_regs, the kernel program=
s an
> > > > > incorrect read shadow value for L2's CR4.
> > > > >=20
> > > > > The result is that the guest can read a value for CR4 where bits =
from
> > > > > L1 have leaked into L2.
> > > >=20
> > > > No, this is a userspace bug.=C2=A0 If L2 is active when userspace s=
tuffs
> > > > register state, then from KVM's perspective the incoming value is L=
2's
> > > > value.=C2=A0 E.g.=C2=A0 if userspace *wants* to update L2 CR4 for w=
hatever
> > > > reason, this patch would result in L2 getting a stale value, i.e. t=
he
> > > > value of CR4 at the time of VM-Enter.
> > > >=20
> > > > And even if userspace wants to change L1, this patch is wrong, as K=
VM
> > > > is writing vmcs02.GUEST_CR4, i.e. is clobbering the L2 CR4 that was
> > > > programmed by L1, *and* is dropping the CR4 value that userspace wa=
nted
> > > > to stuff for L1.
> > > >=20
> > > > To fix this, your userspace needs to either wait until L2 isn't act=
ive,
> > > > or force the vCPU out of L2 (which isn't easy, but it's doable if
> > > > absolutely necessary).
> > >=20
> > > What you say makes sense. Is there any way for
> > > userspace to detect whether L2 is currently active after
> > > returning from KVM_RUN? I couldn't find anything in the official
> > > documentation https://docs.kernel.org/virt/kvm/api.html
> > >=20
> > > Can you point me into the right direction?
> >=20
> > Hmm, the only way to query that information is via KVM_GET_NESTED_STATE=
,
> > which is a bit unfortunate as that is a fairly "heavy" ioctl().

Hur dur, I forgot that KVM provides a "guest_mode" stat.  Userspace can do
KVM_GET_STATS_FD on the vCPU FD to get a file handle to the binary stats, a=
nd
then you wouldn't need to call back into KVM just to query guest_mode.

Ah, and I also forgot that we have kvm_run.flags, so adding KVM_RUN_X86_GUE=
ST_MODE
would also be trivial (I almost suggested it earlier, but didn't want to ad=
d a
new field to kvm_run without a very good reason).

> Indeed. What still does not make sense to me is that userspace should be =
able
> to modify the L2 state. From what I can see, even in this scenario, L0 ex=
its
> with the L1 state.

No, KVM exits with L2.  Or at least, KVM is supposed to exit with L2 state.=
  Exiting
with L1 state would actually be quite difficult, as KVM would need to manua=
lly
switch to vmcs01, pull out state, and then switch back to vmcs02.  And for =
GPRs,
KVM doesn't have L1 state because most GPRs are "volatile", i.e. are clobbe=
red by
VM-Enter and thus need to be manually managed by the hypervisor.

I assume you're using kvm_run.kvm_valid_regs to instruct KVM to save vCPU s=
tate
when exiting to userspace?  That path grabs state straight from the vCPU, w=
ithout
regard to is_guest_mode().  If you're seeing L1 state, then there's likely =
a bug
somewhere, likely in userspace again.   While valid_regs kvm_run.kvm_valid_=
regs
might not be heavily used, the underlying code is very heavily used for doi=
ng
save/restore while L2 is active, e.g. for live migration.

> So what you are saying is that userspace should be allowed to change L2 e=
ven
> if it receives the architectural state from L1?

No, see above.

> What would be the use-case for this scenario?
>=20
> If the above is true, I think we should document this explicitly
> because it's not obvious, at least not for me ;)
>=20
> How would you feel if we extend struct kvm_run with a
> nested_guest_active flag? This should be fairly cheap and would make
> the integration into VirtualBox/KVM much easier. We could also only
> sync this flag explicitly, e.g. with a KVM_SYNC_X86_NESTED_GUEST?

Heh, see above regarding KVM_RUN_X86_GUEST_MODE.