Re: [PATCH 4/4] KVM: x86: lapic: don't allow to set non default apic id when not using x2apic api

[Maxim Levitsky]

On Tue, 2022-03-01 at 16:56 +0000, Sean Christopherson wrote:
> 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..

I know, this series is just set of small fixes - this patch belongs to the series
of my nested avic, but as was requested, I posted it seperately as part of
'fixes only' series, since it is stanadlone. All patches in this series
are standalone.

> 
> 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.

This check is only when CPU is in x2apic mode. In this mode the X86 specs demands that
apic_id == vcpu_id.

When old API is used, APIC IDs are encoded in xapic format, even when vCPU is in x2apic
mode, meaning that apic id can't be more  that 255 even in x2apic mode.
That is why new API 'x2apic_format' was added in first place.

Thus I don't see how this is breaking userspace.

Best regards,
	Maxim Levitsky



> 
> > +
> > +		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
> >