linux-kernel - Re: [PATCH] x86/perf: Fix guest_get_msrs static call if there is no PMU

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CACT4Y+aCP1my-ywPoLTBuSQB1gg-1Ja1M__Xo1W-_EN7PpaAow@mail.gmail.com>
Date:   Mon, 8 Mar 2021 09:51:25 +0100
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Like Xu <like.xu@...ux.intel.com>
Cc:     "Xu, Like" <like.xu@...el.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Jim Mattson <jmattson@...gle.com>,
        KVM list <kvm@...r.kernel.org>,
        "Thomas Gleixner
        (x86/pti/timer/core/smp/irq/perf/efi/locking/ras/objtool)
        (x86@...nel.org)" <tglx@...utronix.de>,
        Borislav Petkov <bp@...en8.de>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Ingo Molnar <mingo@...hat.com>
Subject: Re: [PATCH] x86/perf: Fix guest_get_msrs static call if there is no PMU

On Mon, Mar 8, 2021 at 9:35 AM Like Xu <like.xu@...ux.intel.com> wrote:
>
> On 2021/3/8 15:12, Dmitry Vyukov wrote:
> > On Mon, Mar 8, 2021 at 3:26 AM Xu, Like <like.xu@...el.com> wrote:
> >>
> >> On 2021/3/6 6:33, Sean Christopherson wrote:
> >>> Handle a NULL x86_pmu.guest_get_msrs at invocation instead of patching
> >>> in perf_guest_get_msrs_nop() during setup.  If there is no PMU, setup
> >>
> >> "If there is no PMU" ...
> >>
> >> How to set up this kind of environment,
> >> and what changes are needed in .config or boot parameters ?
> >
> > Hi Xu,
> >
> > This can be reproduced in qemu with "-cpu max,-pmu" flag using this reproducer:
> > https://groups.google.com/g/syzkaller-bugs/c/D8eHw3LIOd0/m/L2G0lVkVBAAJ
>
> Sorry, I couldn't reproduce any VMX abort with "-cpu max,-pmu".
> Doe this patch fix this "unexpected kernel reboot" issue ?
>
> If so, you may add "Tested-by" for more attention.

There is an uninit involved. For me it crashed reliably when kernel
compiled with clang 11, but with gcc it worked most of the time.
You may try to add something like:

--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6581,6 +6581,7 @@ static void atomic_switch_perf_msrs(struct vcpu_vmx *vmx)
        struct perf_guest_switch_msr *msrs;

+      nr_msrs = 12345678;
        msrs = perf_guest_get_msrs(&nr_msrs);
+       pr_err("atomic_switch_perf_msrs: msrs=%px nr_msrs=%d\n", msrs, nr_msrs);

Then you will see surprising things.


> >>> bails before updating the static calls, leaving x86_pmu.guest_get_msrs
> >>> NULL and thus a complete nop.
> >>
> >>> Ultimately, this causes VMX abort on
> >>> VM-Exit due to KVM putting random garbage from the stack into the MSR
> >>> load list.
> >>>
> >>> Fixes: abd562df94d1 ("x86/perf: Use static_call for x86_pmu.guest_get_msrs")
> >>> Cc: Like Xu <like.xu@...ux.intel.com>
> >>> Cc: Paolo Bonzini <pbonzini@...hat.com>
> >>> Cc: Jim Mattson <jmattson@...gle.com>
> >>> Cc: kvm@...r.kernel.org
> >>> Reported-by: Dmitry Vyukov <dvyukov@...gle.com>
> >>> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> >>> ---
> >>>    arch/x86/events/core.c | 16 +++++-----------
> >>>    1 file changed, 5 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> >>> index 6ddeed3cd2ac..ff874461f14c 100644
> >>> --- a/arch/x86/events/core.c
> >>> +++ b/arch/x86/events/core.c
> >>> @@ -671,7 +671,11 @@ void x86_pmu_disable_all(void)
> >>>
> >>>    struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr)
> >>>    {
> >>> -     return static_call(x86_pmu_guest_get_msrs)(nr);
> >>> +     if (x86_pmu.guest_get_msrs)
> >>> +             return static_call(x86_pmu_guest_get_msrs)(nr);
> >>
> >> How about using "static_call_cond" per commit "452cddbff7" ?
> >>
> >>> +
> >>> +     *nr = 0;
> >>> +     return NULL;
> >>>    }
> >>>    EXPORT_SYMBOL_GPL(perf_guest_get_msrs);
> >>>
> >>> @@ -1944,13 +1948,6 @@ static void _x86_pmu_read(struct perf_event *event)
> >>>        x86_perf_event_update(event);
> >>>    }
> >>>
> >>> -static inline struct perf_guest_switch_msr *
> >>> -perf_guest_get_msrs_nop(int *nr)
> >>> -{
> >>> -     *nr = 0;
> >>> -     return NULL;
> >>> -}
> >>> -
> >>>    static int __init init_hw_perf_events(void)
> >>>    {
> >>>        struct x86_pmu_quirk *quirk;
> >>> @@ -2024,9 +2021,6 @@ static int __init init_hw_perf_events(void)
> >>>        if (!x86_pmu.read)
> >>>                x86_pmu.read = _x86_pmu_read;
> >>>
> >>> -     if (!x86_pmu.guest_get_msrs)
> >>> -             x86_pmu.guest_get_msrs = perf_guest_get_msrs_nop;
> >>> -
> >>>        x86_pmu_static_call_update();
> >>>
> >>>        /*
> >>
>