Please, please post standalone patches/fixes as standalone patches/fixes. And in
general, keep series to one topic. There is very real value in following the
established and documented process, e.g. avoids creating artificial dependencies
where a changes works only because of an "unrelated" patch earlier in the series.
And for us reviewers, it helps tremendously as it means I can scan just the cover
letter for a series to prioritize review accordingly. Bundling things together
means I have to scan through every patch to triage the series..
On Tue, Mar 01, 2022, Maxim Levitsky wrote:
> Fix a loop hole in setting the apic state that didn't check if
Heh, "loophole", took me a second to figure out there was no literal loop. :-)
> apic id == vcpu_id when x2apic is enabled but userspace is using
> a older variant of the ioctl which didn't had 32 bit apic ids.
>
> Signed-off-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
> ---
> arch/x86/kvm/lapic.c | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 80a2020c4db40..8d35f56c64020 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2618,15 +2618,14 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu,
> u32 *ldr = (u32 *)(s->regs + APIC_LDR);
> u64 icr;
>
> - if (vcpu->kvm->arch.x2apic_format) {
> - if (*id != vcpu->vcpu_id)
> - return -EINVAL;
> - } else {
> - if (set)
> - *id >>= 24;
> - else
> - *id <<= 24;
> - }
> + if (!vcpu->kvm->arch.x2apic_format && set)
> + *id >>= 24;
> +
> + if (*id != vcpu->vcpu_id)
> + return -EINVAL;
This breaks backwards compability, userspace will start failing where it previously
succeeded. It doesn't even require a malicious userspace, e.g. if userspace creates
a vCPU with a vcpu_id > 255 vCPUs, the shift will truncate and cause failure. It'll
obviously do weird things, but this code is 5.5 years old, I don't think it's worth
trying to close a loophole that doesn't harm KVM.
If we provide a way for userspace to opt into disallowiong APICID != vcpu_id, we
can address this further upstream, e.g. reject vcpu_id > 255 without x2apic_format.
> +
> + if (!vcpu->kvm->arch.x2apic_format && !set)
> + *id <<= 24;
>
> /*
> * In x2APIC mode, the LDR is fixed and based on the id. And
> --
> 2.26.3
>
