linux-kernel - Re: [PATCH] Revert "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] [day] [month] [year] [list]

Date:   Mon, 8 Apr 2019 14:16:10 +0000
From:   "Suthikulpanit, Suravee" <Suravee.Suthikulpanit@....com>
To:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>
CC:     "joro@...tes.org" <joro@...tes.org>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "rkrcmar@...hat.com" <rkrcmar@...hat.com>
Subject: Re: [PATCH] Revert "svm: Fix AVIC incomplete IPI emulation"

Ping

On 3/20/19 3:12 PM, Suthikulpanit, Suravee wrote:
> This reverts commit bb218fbcfaaa3b115d4cd7a43c0ca164f3a96e57.
> 
> As Oren Twaig pointed out the old discussion:
> 
>    https://patchwork.kernel.org/patch/8292231/
> 
> that the change coud potentially cause an extra IPI to be sent to
> the destination vcpu because the AVIC hardware already set the IRR bit
> before the incomplete IPI #VMEXIT with id=1 (target vcpu is not running).
> Since writting to ICR and ICR2 will also set the IRR. If something triggers
> the destination vcpu to get scheduled before the emulation finishes, then
> this could result in an additional IPI.
> 
> Also, the issue mentioned in the commit bb218fbcfaaa was misdiagnosed.
> 
> Cc: Radim Krčmář <rkrcmar@...hat.com>
> Cc: Paolo Bonzini <pbonzini@...hat.com>
> Reported-by: Oren Twaig <oren@...lemp.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@....com>
> ---
>   arch/x86/kvm/svm.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index f13a3a24d360..47c4993448c7 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -4512,14 +4512,25 @@ 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;
>   
>   		/*
> -		 * Update ICR high and low, then emulate sending IPI,
> -		 * which is handled when writing APIC_ICR.
> +		 * 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.
>   		 */
> -		kvm_lapic_reg_write(apic, APIC_ICR2, icrh);
> -		kvm_lapic_reg_write(apic, APIC_ICR, icrl);
> +		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);
> +		}
>   		break;
>   	}
>   	case AVIC_IPI_FAILURE_INVALID_TARGET:
>