| 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
| ||
|
Message-ID: <0591cb18-77e1-4e98-a405-4a39cfb512e1@gmail.com>
Date: Wed, 13 Dec 2023 16:24:58 +0800
From: Like Xu <like.xu.linux@...il.com>
To: Dongli Zhang <dongli.zhang@...cle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>,
Andi Kleen <ak@...ux.intel.com>, kvm@...r.kernel.org,
linux-kernel@...r.kernel.org,
Sean Christopherson <seanjc@...gle.com>
Subject: Re: [PATCH v2] KVM: x86/intr: Explicitly check NMI from guest to
eliminate false positives
On 13/12/2023 3:28 pm, Dongli Zhang wrote:
> Hi Like,
>
> On 12/5/23 19:20, Like Xu wrote:
>> From: Like Xu <likexu@...cent.com>
>>
>> Explicitly checking the source of external interrupt is indeed NMI and not
>> other types in the kvm_arch_pmi_in_guest(), which prevents perf-kvm false
>> positive samples generated in perf/core NMI mode after vm-exit but before
>> kvm_before_interrupt() from being incorrectly labelled as guest samples:
>
> About the before kvm_before_interrupt() ...
>
>>
>> # test: perf-record + cpu-cycles:HP (which collects host-only precise samples)
>> # Symbol Overhead sys usr guest sys guest usr
>> # ....................................... ........ ........ ........ ......... .........
>> #
>> # Before:
>> [g] entry_SYSCALL_64 24.63% 0.00% 0.00% 24.63% 0.00%
>> [g] syscall_return_via_sysret 23.23% 0.00% 0.00% 23.23% 0.00%
>> [g] files_lookup_fd_raw 6.35% 0.00% 0.00% 6.35% 0.00%
>> # After:
>> [k] perf_adjust_freq_unthr_context 57.23% 57.23% 0.00% 0.00% 0.00%
>> [k] __vmx_vcpu_run 4.09% 4.09% 0.00% 0.00% 0.00%
>> [k] vmx_update_host_rsp 3.17% 3.17% 0.00% 0.00% 0.00%
>>
>> In the above case, perf records the samples labelled '[g]', the RIPs behind
>> the weird samples are actually being queried by perf_instruction_pointer()
>> after determining whether it's in GUEST state or not, and here's the issue:
>>
>> If vm-exit is caused by a non-NMI interrupt (such as hrtimer_interrupt) and
>> at least one PMU counter is enabled on host, the kvm_arch_pmi_in_guest()
>> will remain true (KVM_HANDLING_IRQ is set) until kvm_before_interrupt().
>
> ... and here.
>
> Would you mind helping why kvm_arch_pmi_in_guest() remains true before
> *kvm_before_interrupt()*.
>
> According to the source code, the vcpu->arch.handling_intr_from_guest
> is set to non-zero only at kvm_before_interrupt(), and cleared at
> kvm_after_interrupt().
>
> Or would you mean kvm_after_interrupt()?
Oops, it should refer to kvm_after_interrupt() as the code fixed. Thank you.
>
> Thank you very much!
>
> Dongli Zhang
>
>>
>> During this window, if a PMI occurs on host (since the KVM instructions on
>> host are being executed), the control flow, with the help of the host NMI
>> context, will be transferred to perf/core to generate performance samples,
>> thus perf_instruction_pointer() and perf_guest_get_ip() is called.
>>
>> Since kvm_arch_pmi_in_guest() only checks if there is an interrupt, it may
>> cause perf/core to mistakenly assume that the source RIP of the host NMI
>> belongs to the guest world and use perf_guest_get_ip() to get the RIP of
>> a vCPU that has already exited by a non-NMI interrupt.
>>
>> Error samples are recorded and presented to the end-user via perf-report.
>> Such false positive samples could be eliminated by explicitly determining
>> if the exit reason is KVM_HANDLING_NMI.
>>
>> Note that when vm-exit is indeed triggered by PMI and before HANDLING_NMI
>> is cleared, it's also still possible that another PMI is generated on host.
>> Also for perf/core timer mode, the false positives are still possible since
>> that non-NMI sources of interrupts are not always being used by perf/core.
>> In both cases above, perf/core should correctly distinguish between real
>> RIP sources or even need to generate two samples, belonging to host and
>> guest separately, but that's perf/core's story for interested warriors.
>>
>> Fixes: dd60d217062f ("KVM: x86: Fix perf timer mode IP reporting")
>> Signed-off-by: Like Xu <likexu@...cent.com>
>> ---
>> V1 -> V2 Changelog:
>> - Refine commit message to cover both perf/core timer and NMI modes;
>> - Use in_nmi() to distinguish whether it's NMI mode or not; (Sean)
>> V1: https://urldefense.com/v3/__https://lore.kernel.org/kvm/20231204074535.9567-1-likexu@tencent.com/__;!!ACWV5N9M2RV99hQ!MQ8FetD27SVKN34CS_P-K3qrhspFnpf_Mqb0McFN9y5vSUeScc5b0TlZ3ZMDvt4Cn4b3g0h9ci6EO9k3PBEQXpePrg$
>> arch/x86/include/asm/kvm_host.h | 10 +++++++++-
>> arch/x86/kvm/x86.h | 6 ------
>> 2 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index c8c7e2475a18..167d592e08d0 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -1868,8 +1868,16 @@ static inline int kvm_arch_flush_remote_tlbs_range(struct kvm *kvm, gfn_t gfn,
>> }
>> #endif /* CONFIG_HYPERV */
>>
>> +enum kvm_intr_type {
>> + /* Values are arbitrary, but must be non-zero. */
>> + KVM_HANDLING_IRQ = 1,
>> + KVM_HANDLING_NMI,
>> +};
>> +
>> +/* Enable perf NMI and timer modes to work, and minimise false positives. */
>> #define kvm_arch_pmi_in_guest(vcpu) \
>> - ((vcpu) && (vcpu)->arch.handling_intr_from_guest)
>> + ((vcpu) && (vcpu)->arch.handling_intr_from_guest && \
>> + (in_nmi() == ((vcpu)->arch.handling_intr_from_guest == KVM_HANDLING_NMI)))
>>
>> void __init kvm_mmu_x86_module_init(void);
>> int kvm_mmu_vendor_module_init(void);
>> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
>> index 2f7e19166658..4dc38092d599 100644
>> --- a/arch/x86/kvm/x86.h
>> +++ b/arch/x86/kvm/x86.h
>> @@ -431,12 +431,6 @@ static inline bool kvm_notify_vmexit_enabled(struct kvm *kvm)
>> return kvm->arch.notify_vmexit_flags & KVM_X86_NOTIFY_VMEXIT_ENABLED;
>> }
>>
>> -enum kvm_intr_type {
>> - /* Values are arbitrary, but must be non-zero. */
>> - KVM_HANDLING_IRQ = 1,
>> - KVM_HANDLING_NMI,
>> -};
>> -
>> static __always_inline void kvm_before_interrupt(struct kvm_vcpu *vcpu,
>> enum kvm_intr_type intr)
>> {
>>
>> base-commit: 1ab097653e4dd8d23272d028a61352c23486fd4a
Powered by blists - more mailing lists