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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Yh5QJ4dJm63fC42n@google.com>
Date:   Tue, 1 Mar 2022 16:56:07 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Maxim Levitsky <mlevitsk@...hat.com>
Cc:     kvm@...r.kernel.org, Jim Mattson <jmattson@...gle.com>,
        "H. Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Joerg Roedel <joro@...tes.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Borislav Petkov <bp@...en8.de>, x86@...nel.org
Subject: Re: [PATCH 4/4] KVM: x86: lapic: don't allow to set non default apic
 id when not using x2apic api

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@...hat.com>
> ---
>  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
>