linux-kernel - Re: [PATCH v2 3/4] KVM: nVMX: Don't muck with allowed sec exec controls on CPUID changes

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] [day] [month] [year] [list]

Message-ID: <Y7WQYLO4Ou8M8ElV@google.com>
Date:   Wed, 4 Jan 2023 14:42:40 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Aaron Lewis <aaronlewis@...gle.com>,
        Yu Zhang <yu.c.zhang@...ux.intel.com>
Subject: Re: [PATCH v2 3/4] KVM: nVMX: Don't muck with allowed sec exec
 controls on CPUID changes

On Wed, Jan 04, 2023, Sean Christopherson wrote:
> On Fri, Dec 23, 2022, Paolo Bonzini wrote:
> > On 12/13/22 07:23, Sean Christopherson wrote:
> > > Don't modify the set of allowed secondary execution controls, i.e. the
> > > virtual MSR_IA32_VMX_PROCBASED_CTLS2, in response to guest CPUID changes.
> > > To avoid breaking old userspace that never sets the VMX MSRs, i.e. relies
> > > on KVM to provide a consistent vCPU model, keep the existing behavior if
> > > userspace has never written MSR_IA32_VMX_PROCBASED_CTLS2.
> > > 
> > > KVM should not modify the VMX capabilities presented to L1 based on CPUID
> > > as doing so may discard explicit settings provided by userspace.  E.g. if
> > > userspace does KVM_SET_MSRS => KVM_SET_CPUID and disables a feature in
> > > the VMX MSRs but not CPUID (to prevent exposing the feature to L2), then
> > > stuffing the VMX MSRs during KVM_SET_CPUID will expose the feature to L2
> > > against userspace's wishes.
> > 
> > The commit message doesn't explain *why* KVM_SET_CPUID would be done before
> > KVM_SET_MSRS.
> 
> I assume you mean why KVM_SET_MSRS would be done before KVM_SET_CPUID2?
> 
> This patch is mostly paranoia, AFAIK there is no userspace that is negatively
> affected by KVM's manipulations.  The only case I can think of is if userspace
> wanted to emulate dynamic CPUID updates, e.g. set an MSR filter to intercept writes
> to MISC_ENABLES to update MONITOR/MWAIT support, but that behavior isn't allowed
> since commit feb627e8d6f6 ("KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM_RUN").
> 
> There are scenarios where userspace might do KVM_SET_MSRS before KVM_SET_CPUID,
> e.g. QEMU's reuse of a vCPU for CPU hotplug, but in those cases I would expect
> userspace to follow up with another KVM_SET_MSRS.

An argument for taking this patch is that it might be necessary to disallow
KVM_SET_MSRS after KVM_RUN[*].  If KVM manipulates VMX MSRs during KVM_SET_CPUID2,
reusing a vCPU with sequence:

  SET_CPUID2 => SET_MSRS => RUN => unplug => hotplug => SET_CPUID2 => SET_MSRS

sequence will cause the second SET_MSRS to fail due to userspace "changing" the
MSR value.

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