linux-kernel - Re: [PATCH 2/2] KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Fri, 14 Jan 2022 10:31:50 +0100
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Igor Mammedov <imammedo@...hat.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        linux-kernel@...r.kernel.org,
        Sean Christopherson <seanjc@...gle.com>
Subject: Re: [PATCH 2/2] KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM_RUN

Igor Mammedov <imammedo@...hat.com> writes:

> On Thu, 13 Jan 2022 22:33:38 +0000
> Sean Christopherson <seanjc@...gle.com> wrote:
>
>> On Mon, Jan 03, 2022, Igor Mammedov wrote:
>> > On Mon, 03 Jan 2022 09:04:29 +0100
>> > Vitaly Kuznetsov <vkuznets@...hat.com> wrote:
>> >   
>> > > Paolo Bonzini <pbonzini@...hat.com> writes:
>> > >   
>> > > > On 12/27/21 18:32, Igor Mammedov wrote:    
>> > > >>> Tweaked and queued nevertheless, thanks.    
>> > > >> it seems this patch breaks VCPU hotplug, in scenario:
>> > > >> 
>> > > >>    1. hotunplug existing VCPU (QEMU stores VCPU file descriptor in parked cpus list)
>> > > >>    2. hotplug it again (unsuspecting QEMU reuses stored file descriptor when recreating VCPU)
>> > > >> 
>> > > >> RHBZ:https://bugzilla.redhat.com/show_bug.cgi?id=2028337#c11
>> > > >>     
>> > > >
>> > > > The fix here would be (in QEMU) to not call KVM_SET_CPUID2 again. 
>> > > > However, we need to work around it in KVM, and allow KVM_SET_CPUID2 if 
>> > > > the data passed to the ioctl is the same that was set before.    
>> > > 
>> > > Are we sure the data is going to be *exactly* the same? In particular,
>> > > when using vCPU fds from the parked list, do we keep the same
>> > > APIC/x2APIC id when hotplugging? Or can we actually hotplug with a
>> > > different id?  
>> > 
>> > If I recall it right, it can be a different ID easily.  
>> 
>> No, it cannot.  KVM doesn't provide a way for userspace to change the APIC ID of
>> a vCPU after the vCPU is created.  x2APIC flat out disallows changing the APIC ID,
>> and unless there's magic I'm missing, apic_mmio_write() => kvm_lapic_reg_write()
>> is not reachable from userspace.
>> 
>> The only way for userspace to set the APIC ID is to change vcpu->vcpu_id, and that
>> can only be done at KVM_VCPU_CREATE.
>> 
>> So, reusing a parked vCPU for hotplug must reuse the same APIC ID.  QEMU handles
>> this by stashing the vcpu_id, a.k.a. APIC ID, when parking a vCPU, and reuses a
>> parked vCPU if and only if it has the same APIC ID.  And because QEMU derives the
>> APIC ID from topology, that means all the topology CPUID leafs must remain the
>> same, otherwise the guest is hosed because it will send IPIs to the wrong vCPUs.
>
> Indeed, I was wrong.
> I just checked all cpu unplug history in qemu. It was introduced in qemu-2.7
> and from the very beginning it did stash vcpu_id,
> so there is no old QEMU that would re-plug VCPU with different apic_id.
> Though tells us nothing about what other userspace implementations might do.
>

The genie is out of the bottle already, 5.16 is released with the change
(which was promissed for some time, KVM was complaining with
pr_warn_ratelimited()). I'd be brave and say that if QEMU doesn't need
it then nobody else does (out of curiosity, are there KVM VMMs besides
QEMU which support CPU hotplug out there?).

> However, a problem of failing KVM_SET_CPUID2 during VCPU re-plug
> is still there and re-plug will fail if KVM rejects repeated KVM_SET_CPUID2
> even if ioctl called with exactly the same CPUID leafs as the 1st call.
>

Assuming APIC id change doesn not need to be supported, I can send v2
here with an empty allowlist.

-- 
Vitaly