linux-hardening - Re: [PATCH v6 1/4] powerpc/64s: Introduce temporary mm for Radix MMU

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: <CACzsE9rThU0JBACJoeeHOyEOA8CbFwRExrOhTsySaOH44yJa6g@mail.gmail.com>
Date:   Sat, 11 Sep 2021 18:26:32 +1000
From:   Jordan Niethe <jniethe5@...il.com>
To:     "Christopher M. Riedl" <cmr@...escreens.de>
Cc:     linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH v6 1/4] powerpc/64s: Introduce temporary mm for Radix MMU

On Sat, Sep 11, 2021 at 12:35 PM Christopher M. Riedl
<cmr@...escreens.de> wrote:
>
> x86 supports the notion of a temporary mm which restricts access to
> temporary PTEs to a single CPU. A temporary mm is useful for situations
> where a CPU needs to perform sensitive operations (such as patching a
> STRICT_KERNEL_RWX kernel) requiring temporary mappings without exposing
> said mappings to other CPUs. Another benefit is that other CPU TLBs do
> not need to be flushed when the temporary mm is torn down.
>
> Mappings in the temporary mm can be set in the userspace portion of the
> address-space.
>
> Interrupts must be disabled while the temporary mm is in use. HW
> breakpoints, which may have been set by userspace as watchpoints on
> addresses now within the temporary mm, are saved and disabled when
> loading the temporary mm. The HW breakpoints are restored when unloading
> the temporary mm. All HW breakpoints are indiscriminately disabled while
> the temporary mm is in use - this may include breakpoints set by perf.

I had thought CPUs with a DAWR might not need to do this because the
privilege level that breakpoints trigger on can be configured. But it
turns out in ptrace, etc we use HW_BRK_TYPE_PRIV_ALL.

>
> Based on x86 implementation:
>
> commit cefa929c034e
> ("x86/mm: Introduce temporary mm structs")
>
> Signed-off-by: Christopher M. Riedl <cmr@...escreens.de>
>
> ---
>
> v6:  * Use {start,stop}_using_temporary_mm() instead of
>        {use,unuse}_temporary_mm() as suggested by Christophe.
>
> v5:  * Drop support for using a temporary mm on Book3s64 Hash MMU.
>
> v4:  * Pass the prev mm instead of NULL to switch_mm_irqs_off() when
>        using/unusing the temp mm as suggested by Jann Horn to keep
>        the context.active counter in-sync on mm/nohash.
>      * Disable SLB preload in the temporary mm when initializing the
>        temp_mm struct.
>      * Include asm/debug.h header to fix build issue with
>        ppc44x_defconfig.
> ---
>  arch/powerpc/include/asm/debug.h |  1 +
>  arch/powerpc/kernel/process.c    |  5 +++
>  arch/powerpc/lib/code-patching.c | 56 ++++++++++++++++++++++++++++++++
>  3 files changed, 62 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/debug.h b/arch/powerpc/include/asm/debug.h
> index 86a14736c76c..dfd82635ea8b 100644
> --- a/arch/powerpc/include/asm/debug.h
> +++ b/arch/powerpc/include/asm/debug.h
> @@ -46,6 +46,7 @@ static inline int debugger_fault_handler(struct pt_regs *regs) { return 0; }
>  #endif
>
>  void __set_breakpoint(int nr, struct arch_hw_breakpoint *brk);
> +void __get_breakpoint(int nr, struct arch_hw_breakpoint *brk);
>  bool ppc_breakpoint_available(void);
>  #ifdef CONFIG_PPC_ADV_DEBUG_REGS
>  extern void do_send_trap(struct pt_regs *regs, unsigned long address,
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 50436b52c213..6aa1f5c4d520 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -865,6 +865,11 @@ static inline int set_breakpoint_8xx(struct arch_hw_breakpoint *brk)
>         return 0;
>  }
>
> +void __get_breakpoint(int nr, struct arch_hw_breakpoint *brk)
> +{
> +       memcpy(brk, this_cpu_ptr(&current_brk[nr]), sizeof(*brk));
> +}

The breakpoint code is already a little hard to follow. I'm worried
doing this might spread breakpoint handling into more places in the
future.
What about something like having a breakpoint_pause() function which
clears the hardware registers only and then a breakpoint_resume()
function that copies from current_brk[] back to the hardware
registers?
Then we don't have to make another copy of the breakpoint state.

> +
>  void __set_breakpoint(int nr, struct arch_hw_breakpoint *brk)
>  {
>         memcpy(this_cpu_ptr(&current_brk[nr]), brk, sizeof(*brk));
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index f9a3019e37b4..8d61a7d35b89 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c

Sorry I might have missed it, but what was the reason for not putting
this stuff in mmu_context.h?

> @@ -17,6 +17,9 @@
>  #include <asm/code-patching.h>
>  #include <asm/setup.h>
>  #include <asm/inst.h>
> +#include <asm/mmu_context.h>
> +#include <asm/debug.h>
> +#include <asm/tlb.h>
>
>  static int __patch_instruction(u32 *exec_addr, struct ppc_inst instr, u32 *patch_addr)
>  {
> @@ -45,6 +48,59 @@ int raw_patch_instruction(u32 *addr, struct ppc_inst instr)
>  }
>
>  #ifdef CONFIG_STRICT_KERNEL_RWX
> +
> +struct temp_mm {
> +       struct mm_struct *temp;
> +       struct mm_struct *prev;
> +       struct arch_hw_breakpoint brk[HBP_NUM_MAX];
^ Then we wouldn't need this.
> +};
> +
> +static inline void init_temp_mm(struct temp_mm *temp_mm, struct mm_struct *mm)
> +{
> +       /* We currently only support temporary mm on the Book3s64 Radix MMU */
> +       WARN_ON(!radix_enabled());
> +
> +       temp_mm->temp = mm;
> +       temp_mm->prev = NULL;
> +       memset(&temp_mm->brk, 0, sizeof(temp_mm->brk));
> +}
> +
> +static inline void start_using_temporary_mm(struct temp_mm *temp_mm)
> +{
> +       lockdep_assert_irqs_disabled();
> +
> +       temp_mm->prev = current->active_mm;
> +       switch_mm_irqs_off(temp_mm->prev, temp_mm->temp, current);

Now that we only support radix it should be fine again to have it like this:
switch_mm_irqs_off(NULL, temp_mm->temp, current);?
It was changed from that because it would cause issues on nohash I thought.

> +
> +       WARN_ON(!mm_is_thread_local(temp_mm->temp));
> +
> +       if (ppc_breakpoint_available()) {
> +               struct arch_hw_breakpoint null_brk = {0};
> +               int i = 0;
> +
> +               for (; i < nr_wp_slots(); ++i) {
> +                       __get_breakpoint(i, &temp_mm->brk[i]);
> +                       if (temp_mm->brk[i].type != 0)
> +                               __set_breakpoint(i, &null_brk);
> +               }
> +       }
> +}
> +
> +static inline void stop_using_temporary_mm(struct temp_mm *temp_mm)
> +{
> +       lockdep_assert_irqs_disabled();
> +
> +       switch_mm_irqs_off(temp_mm->temp, temp_mm->prev, current);
> +
> +       if (ppc_breakpoint_available()) {
> +               int i = 0;
> +
> +               for (; i < nr_wp_slots(); ++i)
> +                       if (temp_mm->brk[i].type != 0)
> +                               __set_breakpoint(i, &temp_mm->brk[i]);
> +       }
> +}
> +
>  static DEFINE_PER_CPU(struct vm_struct *, text_poke_area);
>
>  static int text_area_cpu_up(unsigned int cpu)
> --
> 2.32.0
>
Thanks,
Jordan