linux-kernel - Re: [PATCH] svm: Fix AVIC incomplete IPI emulation

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]

Message-ID: <d5b38b30-43e2-7220-6f51-c087a74c37ec@scalemp.com>
Date:   Fri, 8 Mar 2019 23:56:02 +0200
From:   Oren Twaig <oren@...lemp.com>
To:     suravee.suthikulpanit@....com
Cc:     kvm@...r.kernel.org, x86@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] svm: Fix AVIC incomplete IPI emulation

Hi,


Any help appreciated..


Thanks,

Oren Twaig

On 3/4/2019 8:15 PM, Oren Twaig wrote:
> Hello Suravee,
>
> According to AMD's SDM, the target-not-running incomplete
> ipi exit is only received if any of the destination cpus had the
>
> not-running bit set in the avic backing page. However, not
>
> before the CPU _already_ set the relevant IRR bit in all these cpus.
>
>
> In this change, the patch forces KVM to send another interrupt
>
> to the vcpu whether SVM already did that or not. Which means
>
> the vcpu/s, under some conditions, can get an EXTRA interrupt
>
> it never intended to get.
>
>
> Example:
>   1. vcpu B: Is in "not-running" state.
>   2. vcpu A: Writes to the ICR to send vector 80 to vcpu B
>   3. vcpu A: SVM updates vcpu B IRR with bit 80
>   4. vcpu A: SVM exits on incomplete IPI target-not-running exit.
>   5. vcpu A: Now stops executing any code @ hypervisor level.
>   6. vcpu B: Due to another interrupt (like lapic timer)
>      resumes running the guest. While handling interrupts,
>      it also handles interrupt vector 80 (as it's in his IRR)
>   7. vcpu A: resumes executing the below code and sends
>      an _additional_interrupt to vcpu B.
>
> Overall, vcpu B got two interrupts. The second is unwanted and
> not documented in the system architecture.
>
> Can you please elaborate more to why the implementation
>
> below conflict with the specifications (which was the code
>
> before this commit) ?
>
>
> Thanks,
> Oren Twaig
>
>
> > From    "Suthikulpanit, Suravee" <>
> > Subject    [PATCH] svm: Fix AVIC incomplete IPI emulation
> > Date    Tue, 22 Jan 2019 10:25:13 +0000
> > share
> > From: Suravee Suthikulpanit <suravee.suthikulpanit@....com>
> >
> > In case of incomplete IPI with invalid interrupt type, the current
> > SVM driver does not properly emulate the IPI, and fails to boot
> > FreeBSD guests with multiple vcpus when enabling AVIC.
> >
> > Fix this by update APIC ICR high/low registers, which also
> > emulate sending the IPI.
> >
> > Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@....com>
> > ---
> > arch/x86/kvm/svm.c | 19 ++++---------------
> > 1 file changed, 4 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > index 2aff835a65ed..8a0c9a1f6ac8 100644
> > --- a/arch/x86/kvm/svm.c
> > +++ b/arch/x86/kvm/svm.c
> > @@ -4504,25 +4504,14 @@ static int 
> avic_incomplete_ipi_interception(struct vcpu_svm *svm)
> >          kvm_lapic_reg_write(apic, APIC_ICR, icrl);
> >          break;
> >      case AVIC_IPI_FAILURE_TARGET_NOT_RUNNING: {
> > -        int i;
> > -        struct kvm_vcpu *vcpu;
> > -        struct kvm *kvm = svm->vcpu.kvm;
> >          struct kvm_lapic *apic = svm->vcpu.arch.apic;
> >
>          /*
> > -         * At this point, we expect that the AVIC HW has already
> > -         * set the appropriate IRR bits on the valid target
> > -         * vcpus. So, we just need to kick the appropriate vcpu.
> > +         * Update ICR high and low, then emulate sending IPI,
> > +         * which is handled when writing APIC_ICR.
> >           */
> > -        kvm_for_each_vcpu(i, vcpu, kvm) {
> > -            bool m = kvm_apic_match_dest(vcpu, apic,
> > -                             icrl & KVM_APIC_SHORT_MASK,
> > - GET_APIC_DEST_FIELD(icrh),
> > -      icrl & KVM_APIC_DEST_MASK);
> > -
> > -            if (m && !avic_vcpu_is_running(vcpu))
> > - kvm_vcpu_wake_up(vcpu);
> > -        }
> > +        kvm_lapic_reg_write(apic, APIC_ICR2, icrh);
> > +        kvm_lapic_reg_write(apic, APIC_ICR, icrl);
> >          break;
> >      }
> >      case AVIC_IPI_FAILURE_INVALID_TARGET:
> > --
> > 2.17.1
>