Re: [PATCH v2 2/2] KVM: Inject #GP on invalid writes to x2APIC registers

[Sean Christopherson <seanjc@google.com>]

On Wed, May 25, 2022, Venkatesh Srinivas wrote:
> From: Marc Orr <marcorr@google.com>
> 
> From: Venkatesh Srinivas <venkateshs@chromium.org>

Heh, something is wonky in your setup, only Marc should get an explicit From:

> The upper bytes of any x2APIC register are reserved. Inject a #GP
> into the guest if any of these reserved bits are set.
> 
> Signed-off-by: Marc Orr <marcorr@google.com>
> Signed-off-by: Venkatesh Srinivas <venkateshs@chromium.org>
> ---
>  arch/x86/kvm/lapic.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 6f8522e8c492..617e4936c5cc 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2907,6 +2907,8 @@ int kvm_x2apic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>  
>  	if (!lapic_in_kernel(vcpu) || !apic_x2apic_mode(apic))
>  		return 1;
> +	else if (data >> 32)
> +		return 1;

This is incorrect, ICR is a 64-bit value.  Marc's changelog for the internal patch
was wrong, although the code was correct.

The correct upstream change is:

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 39b805666a18..54d0f350acdf 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2892,6 +2892,9 @@ static int kvm_lapic_msr_write(struct kvm_lapic *apic, u32 reg, u64 data)
        if (reg == APIC_ICR)
                return kvm_x2apic_icr_write(apic, data);

+       if (data >> 32)
+               return 1;
+
        return kvm_lapic_reg_write(apic, reg, (u32)data);
 }

As penance for not testing, can you write a KUT testcase to hit all the registers?
You could use e.g. SIPI to test that a 64-bit ICR value is _not_ rejected.

One thought would be to base your testcase on top of similar changes to msr.c to
blast all of the MCE MSRs, e.g. add a path/helper to enable x2APIC and iterate over
all registers.

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