Re: [PATCH v2 1/7] KVM: nVMX: Keep KVM updates to BNDCFGS ctrl bits across MSR write

[Oliver Upton <oupton@google.com>]

Hi Paolo,

On Mon, Feb 07, 2022 at 06:21:30PM +0100, Paolo Bonzini wrote:
> On 2/4/22 21:46, Oliver Upton wrote:
> > Since commit 5f76f6f5ff96 ("KVM: nVMX: Do not expose MPX VMX controls
> > when guest MPX disabled"), KVM has taken ownership of the "load
> > IA32_BNDCFGS" and "clear IA32_BNDCFGS" VMX entry/exit controls. The ABI
> > is that these bits must be set in the IA32_VMX_TRUE_{ENTRY,EXIT}_CTLS
> > MSRs if the guest's CPUID supports MPX, and clear otherwise.
> > 
> > However, KVM will only do so if userspace sets the CPUID before writing
> > to the corresponding MSRs. Of course, there are no ordering requirements
> > between these ioctls. Uphold the ABI regardless of ordering by
> > reapplying KVMs tweaks to the VMX control MSRs after userspace has
> > written to them.
> 
> I don't understand this patch.  If you first write the CPUID and then the
> MSR, the consistency is upheld by these checks:
> 
>         if (!is_bitwise_subset(data, supported, GENMASK_ULL(31, 0)))
>                 return -EINVAL;
> 
>         if (!is_bitwise_subset(supported, data, GENMASK_ULL(63, 32)))
>                 return -EINVAL;

Right, this works if KVM chose to clear the bit, but userspace is trying
to set it. If KVM chose to set the bit, and userspace attempts to clear
it, these checks would pass.

> If you're fixing a case where userspace first writes the MSR and then sets
> CPUID, then I would expect that KVM_SET_CPUID2 redoes those checks and, if
> they fail, it adjusts the MSRs.

I am fixing the case where userspace issues KVM_SET_CPUID2 then writes
to the corresponding MSRs.

Until recently, this all sort of 'worked'. Since we called
kvm_update_cpuid() all the time it was possible for KVM to overwrite the
bits after the MSR write, just not immediately so. After the whole CPUID
rework, we only update the VMX control MSRs immediately after a
KVM_SET_CPUID2, meaning we've missed the case of MSR write after CPUID.

The entire spirit of this series is to back KVM out of touching these
bits, but it seemingly requires a quirk to do so given the userspace
expectations around these bits. Given that, these first two patches fix
the ordering issue between KVM_SET_CPUID2 and an MSR write and enforce
KVM ownership unconditionally.

--
Thanks,
Oliver