linux-kernel - Re: [PATCH 2/3] KVM: x86: Use actual kvm_cpuid.base for clearing KVM_FEATURE_PV

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: <ZeDS2nhkK_QDBJS0@google.com>
Date: Thu, 29 Feb 2024 10:54:18 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Vitaly Kuznetsov <vkuznets@...hat.com>
Cc: kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>, 
	Li RongQing <lirongqing@...du.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] KVM: x86: Use actual kvm_cpuid.base for clearing KVM_FEATURE_PV_UNHALT

On Thu, Feb 29, 2024, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@...gle.com> writes:
> > Am I missing something, or can we just swap() the new and old, update the new
> > in the context of the vCPU, and then undo the swap() if there's an issue?
> > vcpu->mutex is held, and accessing this state from a different task is wildly
> > unsafe, so I don't see any problem with temporarily having an in-flux state.
> >
> 
> I don't see why this approach shouldn't work and I agree it looks like
> it would make things better but I can't say that I'm in love with
> it.

Agreed, but the lack of atomicity is a pre-existing problem, though as proposed,
my idea would make it worse.  More below.

> Ideally, I would want to see the following "atomic" workflow for all
> updates:
> 
> - Check that the supplied data is correct, return an error if not. No
> changes to the state on this step.
> - Tweak the data if needed.
> - Update the state and apply the side-effects of the update. Ideally,
> there should be no errors on this step as rollback can be
> problemmatic. In the real world we will have to handle e.g. failed
> memory allocations here but in most cases the best course of action is
> to kill the VM.
> 
> Well, kvm_set_cpuid() is not like that. At least:
> - kvm_hv_vcpu_init() is a side-effect but we apply it before all checks
> are complete. There's no way back.
> - kvm_check_cpuid() sounds like a pure checker but in reallity we end up
> mangling guest FPU state in fpstate_realloc()

Yeah, I really, really don't like the call to fpu_enable_guest_xfd_features().
But to not make it worse, that call could be hoisted out of kvm_check_cpuid()
so that it can be performed after kvm_cpuid_check_equal(), i.e. be kept dead last
(and with a comment saying it needs to be dead last due to side effects that are
visible to serspace).

> Both are probably "no big deal" but certainly break the atomicity.
>
> > If we want to be paranoid, we can probably get away with killing the VM if the
> > vCPU has run and the incoming CPUID is "bad", e.g. to guard against something
> > in kvm_set_cpuid() consuming soon-to-be-stale state.  And that's actually a
> > feature of sorts, because _if_ something in kvm_set_cpuid() consumes the vCPU's
> > CPUID, then we have a bug _now_ that affects the happy path.
> >
> > Completely untested (I haven't updated the myriad helpers), but this would allow
> > us to revert/remove all of the changes that allow peeking at a CPUID array that
> > lives outside of the vCPU.
> 
> Thanks, assuming there's no urgency

Definitely no urgency.

> let me take a look at this in the course of the next week or so.

No need, it was more of an "FYI, this is what I may go futz with".  Specifically,
it will impact what I want to do with guest cpu_caps[*], hopefully in a good way.
My plan is to play around with it when I get back to that series.

[*] https://lore.kernel.org/all/20231110235528.1561679-1-seanjc@google.com