linux-kernel - Re: [PATCH RESEND v3 03/11] KVM: x86/pmu: Protect kvm->arch.pmu_event

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: <CALMp9eSp5d3mq2GaT+AhDSXaZ=HN0SUSnW5VGY+o4NHLz=VkSA@mail.gmail.com>
Date:   Fri, 20 May 2022 06:00:35 -0700
From:   Jim Mattson <jmattson@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Like Xu <like.xu.linux@...il.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Joerg Roedel <joro@...tes.org>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Subject: Re: [PATCH RESEND v3 03/11] KVM: x86/pmu: Protect kvm->arch.pmu_event_filter
 with SRCU

On Fri, May 20, 2022 at 5:51 AM Paolo Bonzini <pbonzini@...hat.com> wrote:
>
> On 5/18/22 15:25, Like Xu wrote:
> > From: Like Xu <likexu@...cent.com>
> >
> > Similar to "kvm->arch.msr_filter", KVM should guarantee that vCPUs will
> > see either the previous filter or the new filter when user space calls
> > KVM_SET_PMU_EVENT_FILTER ioctl with the vCPU running so that guest
> > pmu events with identical settings in both the old and new filter have
> > deterministic behavior.
> >
> > Fixes: 66bb8a065f5a ("KVM: x86: PMU Event Filter")
> > Signed-off-by: Like Xu <likexu@...cent.com>
> > Reviewed-by: Wanpeng Li <wanpengli@...cent.com>
>
> Please always include the call trace where SRCU is not taken.  The ones
> I reconstructed always end up at a place inside srcu_read_lock/unlock:
>
> reprogram_gp_counter/reprogram_fixed_counter
>    amd_pmu_set_msr
>     kvm_set_msr_common
>      svm_set_msr
>       __kvm_set_msr
>       kvm_set_msr_ignored_check
>        kvm_set_msr_with_filter
>         kvm_emulate_wrmsr**
>         emulator_set_msr_with_filter**
>        kvm_set_msr
>         emulator_set_msr**
>        do_set_msr
>         __msr_io
>          msr_io
>           ioctl(KVM_SET_MSRS)**
>    intel_pmu_set_msr
>     kvm_set_msr_common
>      vmx_set_msr (see svm_set_msr)
>    reprogram_counter
>     global_ctrl_changed
>      intel_pmu_set_msr (see above)
>     kvm_pmu_handle_event
>      vcpu_enter_guest**
>     kvm_pmu_incr_counter
>      kvm_pmu_trigger_event
>       nested_vmx_run**
>       kvm_skip_emulated_instruction**
>       x86_emulate_instruction**
>    reprogram_fixed_counters
>     intel_pmu_set_msr (see above)
>
> Paolo

I agree with Paolo that existing usage is covered by
srcu_read_lock/unlock, but (a) it's not easy to confirm this, and (b)
this is very fragile.

Whichever way we decide to go, the userspace MSR filter and the PMU
event filter should adopt the same approach.