Re: [PATCH 3/3] KVM: selftests: Add test for race in kvm_recalculate_apic_map()

[Sean Christopherson <seanjc@google.com>]

On Sun, May 28, 2023, Michal Luczaj wrote:
> On 5/27/23 01:40, Sean Christopherson wrote:
> > All of these open coded ioctl() calls is unnecessary.  Ditto for the fancy
> > stuffing, which through trial and error I discovered is done to avoid having
> > vCPUs with aliased xAPIC IDs, which would cause KVM to bail before triggering
> > the bug.  It's much easier to just create the max number of vCPUs and enable
> > x2APIC on all of them.
> > ...
> 
> Yup, this looks way better, thanks.
> (FWIW, the #defines were deliberately named to match enum lapic_mode.)

I figured as much, but I find enum lapic_mode to be rather odd, and if that thing
ever gets cleaned up I'd prefer not to have to go fixup selftests too.

> In somewhat related news, I've hit kvm_recalculate_logical_map()'s
> WARN_ON_ONCE(ldr != kvm_apic_calc_x2apic_ldr(kvm_x2apic_id(apic))):

...

> diff --git a/tools/testing/selftests/kvm/x86_64/recalc_apic_map_warn.c b/tools/testing/selftests/kvm/x86_64/recalc_apic_map_warn.c
> new file mode 100644
> index 000000000000..2845e1d9b865
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86_64/recalc_apic_map_warn.c
> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * recalc_apic_map_warn
> + *
> + * Test for hitting WARN_ON_ONCE() in kvm_recalculate_logical_map().
> + */
> +
> +#include "processor.h"
> +#include "kvm_util.h"
> +#include "apic.h"
> +
> +#define	LAPIC_X2APIC	(MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE)
> +
> +int main(void)
> +{
> +	struct kvm_lapic_state lapic = {};
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_vm *vm;
> +
> +	vm = vm_create_with_one_vcpu(&vcpu, NULL); /* vcpu_id = 0 */
> +	vcpu_set_msr(vcpu, MSR_IA32_APICBASE, LAPIC_X2APIC);
> +
> +	*(u32 *)(lapic.regs + APIC_ID) = 1 << 24; /* != vcpu_id */
> +	*(u32 *)(lapic.regs + APIC_SPIV) = APIC_SPIV_APIC_ENABLED;
> +	vcpu_ioctl(vcpu, KVM_SET_LAPIC, &lapic);

Blech.  There's a semi-known backdoor that lets userspace modify the x2APIC ID
and thus the LDR.  Sort of.  KVM doesn't actually honor the modified ID except
for guest RDMSR, e.g. KVM will never deliver interrupts to the unexpected ID.

I'll send a patch (plus this test) to close that loophole, the odds of breaking
an existing setup are basically nil, and it would be very nice to make the x2APIC
ID (and LDR) fully readonly.