linux-kernel - Re: [PATCH] powerpc/perf: Dereference bhrb entries safely

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: <CAKTCnzmuwkBLxaFpROo7TTcky970w7s14ETacHseL6Vu1JPTQA@mail.gmail.com>
Date:   Thu, 14 Dec 2017 23:24:10 +1100
From:   Balbir Singh <bsingharora@...il.com>
To:     Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>
Cc:     Michael Ellerman <mpe@...erman.id.au>,
        Madhavan Srinivasan <maddy@...ux.vnet.ibm.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Kamalesh Babulal <kamalesh@...ux.vnet.ibm.com>,
        Paul Mackerras <paulus@...ba.org>, kan.liang@...el.com,
        Thomas Gleixner <tglx@...utronix.de>,
        "open list:LINUX FOR POWERPC (32-BIT AND 64-BIT)" 
        <linuxppc-dev@...ts.ozlabs.org>,
        "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>
Subject: Re: [PATCH] powerpc/perf: Dereference bhrb entries safely

On Tue, Dec 12, 2017 at 11:29 PM, Ravi Bangoria
<ravi.bangoria@...ux.vnet.ibm.com> wrote:
> It may very well happen that branch instructions recorded by
> bhrb entries already get unmapped before they get processed by
> the kernel. Hence, trying to dereference such memory location
> will endup in a crash. Ex,
>
>     Unable to handle kernel paging request for data at address 0xc008000019c41764
>     Faulting instruction address: 0xc000000000084a14
>     NIP [c000000000084a14] branch_target+0x4/0x70
>     LR [c0000000000eb828] record_and_restart+0x568/0x5c0
>     Call Trace:
>     [c0000000000eb3b4] record_and_restart+0xf4/0x5c0 (unreliable)
>     [c0000000000ec378] perf_event_interrupt+0x298/0x460
>     [c000000000027964] performance_monitor_exception+0x54/0x70
>     [c000000000009ba4] performance_monitor_common+0x114/0x120
>
> Fix this by deferefencing them safely.
>
> Suggested-by: Naveen N. Rao <naveen.n.rao@...ux.vnet.ibm.com>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>
> ---
>  arch/powerpc/perf/core-book3s.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index 9e3da168d54c..5a68d2effdf9 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -410,8 +410,11 @@ static __u64 power_pmu_bhrb_to(u64 addr)
>         int ret;
>         __u64 target;
>
> -       if (is_kernel_addr(addr))
> -               return branch_target((unsigned int *)addr);
> +       if (is_kernel_addr(addr)) {

I think __kernel_text_address() is more accurate right? In which case
you need to check for is_kernel_addr(addr) and if its not kernel_text_address()
then we have an interesting case of a branch from something not text.
It would be nice to catch such cases.

> +               if (probe_kernel_address((void *)addr, instr))
> +                       return 0;
> +               return branch_target(&instr);
> +       }
>
>         /* Userspace: need copy instruction here then translate it */
>         pagefault_disable();

Otherwise,

Reviewed-by: Balbir Singh <bsingharora@...il.com>