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: <CAEf4BzZx5iCAwS0iZv_0S1cADw840S8Ra=_PHPZhnW9cOYhFOQ@mail.gmail.com>
Date: Mon, 30 Sep 2024 13:58:01 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Alexei Starovoitov <alexei.starovoitov@...il.com>, Andrii Nakryiko <andrii@...nel.org>, 
	Jiri Olsa <jolsa@...nel.org>, X86 ML <x86@...nel.org>, LKML <linux-kernel@...r.kernel.org>, 
	alyssa.milburn@...el.com, scott.d.constable@...el.com, 
	Joao Moreira <joao@...rdrivepizza.com>, Andrew Cooper <andrew.cooper3@...rix.com>, 
	Josh Poimboeuf <jpoimboe@...nel.org>, "Jose E. Marchesi" <jose.marchesi@...cle.com>, 
	"H.J. Lu" <hjl.tools@...il.com>, Nick Desaulniers <ndesaulniers@...gle.com>, 
	Sami Tolvanen <samitolvanen@...gle.com>, Nathan Chancellor <nathan@...nel.org>, ojeda@...nel.org, 
	Kees Cook <kees@...nel.org>
Subject: Re: [PATCH 07/14] x86/ibt: Clean up is_endbr()

On Mon, Sep 30, 2024 at 2:33 AM Peter Zijlstra <peterz@...radead.org> wrote:
>
> On Mon, Sep 30, 2024 at 10:30:26AM +0200, Peter Zijlstra wrote:
> > On Sun, Sep 29, 2024 at 10:32:38AM -0700, Alexei Starovoitov wrote:
> > > On Fri, Sep 27, 2024 at 12:50 PM Peter Zijlstra <peterz@...radead.org> wrote:
> >
> > > > --- a/kernel/trace/bpf_trace.c
> > > > +++ b/kernel/trace/bpf_trace.c
> > > > @@ -1027,19 +1027,9 @@ static const struct bpf_func_proto bpf_g
> > > >  #ifdef CONFIG_X86_KERNEL_IBT
> > > >  static unsigned long get_entry_ip(unsigned long fentry_ip)
> > > >  {
> > > > -       u32 instr;
> > > > +       if (is_endbr((void *)(fentry_ip - ENDBR_INSN_SIZE)))
> > > > +               return fentry_ip - ENDBR_INSN_SIZE;
> > > >
> > > > -       /* We want to be extra safe in case entry ip is on the page edge,
> > > > -        * but otherwise we need to avoid get_kernel_nofault()'s overhead.
> > > > -        */
> > > > -       if ((fentry_ip & ~PAGE_MASK) < ENDBR_INSN_SIZE) {
> > > > -               if (get_kernel_nofault(instr, (u32 *)(fentry_ip - ENDBR_INSN_SIZE)))
> > > > -                       return fentry_ip;
> > > > -       } else {
> > > > -               instr = *(u32 *)(fentry_ip - ENDBR_INSN_SIZE);
> > > > -       }
> > > > -       if (is_endbr(instr))
> > > > -               fentry_ip -= ENDBR_INSN_SIZE;
> > > >         return fentry_ip;
> > >
> > > Pls don't.
> > >
> > > This re-introduces the overhead that we want to avoid.
> > >
> > > Just call __is_endbr() here and keep the optimization.
> >
> > Well, I could do that ofcourse, but as I wrote elsewhere, the right
> > thing to do is to optimize get_kernel_nofault(), its current
> > implementation is needlessly expensive. All we really need is a load
> > with an exception entry, the STAC/CLAC and speculation nonsense should
> > not be needed.
>
> Looking at things, something like the below actually generates sane
> code:
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index a582cd25ca87..84f65ee9736c 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1029,17 +1029,10 @@ static unsigned long get_entry_ip(unsigned long fentry_ip)
>  {
>         u32 instr;
>
> -       /* We want to be extra safe in case entry ip is on the page edge,
> -        * but otherwise we need to avoid get_kernel_nofault()'s overhead.
> -        */
> -       if ((fentry_ip & ~PAGE_MASK) < ENDBR_INSN_SIZE) {
> -               if (get_kernel_nofault(instr, (u32 *)(fentry_ip - ENDBR_INSN_SIZE)))
> -                       return fentry_ip;
> -       } else {
> -               instr = *(u32 *)(fentry_ip - ENDBR_INSN_SIZE);
> -       }
> +       __get_kernel_nofault(&instr, (u32 *)(fentry_ip - ENDBR_INSN_SIZE), u32, Efault);
>         if (is_endbr(instr))
>                 fentry_ip -= ENDBR_INSN_SIZE;
> +Efault:
>         return fentry_ip;
>  }
>  #else
>
>
> Which then leads to me rewriting the proposed patch as...
>
> ---
> Subject: x86/ibt: Clean up is_endbr()
> From: Peter Zijlstra <peterz@...radead.org>
>
> Pretty much every caller of is_endbr() actually wants to test something at an
> address and ends up doing get_kernel_nofault(). Fold the lot into a more
> convenient helper.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> ---
>  arch/x86/events/core.c         |    2 +-
>  arch/x86/include/asm/ibt.h     |    5 +++--
>  arch/x86/kernel/alternative.c  |   20 ++++++++++++++------
>  arch/x86/kernel/kprobes/core.c |   11 +----------
>  arch/x86/net/bpf_jit_comp.c    |    4 ++--
>  kernel/trace/bpf_trace.c       |   14 ++------------
>  6 files changed, 23 insertions(+), 33 deletions(-)
>
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -2845,7 +2845,7 @@ static bool is_uprobe_at_func_entry(stru
>                 return true;
>
>         /* endbr64 (64-bit only) */
> -       if (user_64bit_mode(regs) && is_endbr(*(u32 *)auprobe->insn))
> +       if (user_64bit_mode(regs) && is_endbr((u32 *)auprobe->insn))
>                 return true;
>
>         return false;
> --- a/arch/x86/include/asm/ibt.h
> +++ b/arch/x86/include/asm/ibt.h
> @@ -65,7 +65,7 @@ static __always_inline __attribute_const
>         return 0x001f0f66; /* osp nopl (%rax) */
>  }
>
> -static inline bool is_endbr(u32 val)
> +static inline bool __is_endbr(u32 val)
>  {
>         if (val == gen_endbr_poison())
>                 return true;
> @@ -74,6 +74,7 @@ static inline bool is_endbr(u32 val)
>         return val == gen_endbr();
>  }
>
> +extern __noendbr bool is_endbr(u32 *val);
>  extern __noendbr u64 ibt_save(bool disable);
>  extern __noendbr void ibt_restore(u64 save);
>
> @@ -102,7 +103,7 @@ extern bool __do_kernel_cp_fault(struct
>
>  #define __noendbr
>
> -static inline bool is_endbr(u32 val) { return false; }
> +static inline bool is_endbr(u32 *val) { return false; }
>
>  static inline u64 ibt_save(bool disable) { return 0; }
>  static inline void ibt_restore(u64 save) { }
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -852,16 +852,24 @@ void __init_or_module noinline apply_ret
>
>  #ifdef CONFIG_X86_KERNEL_IBT
>
> +__noendbr bool is_endbr(u32 *val)
> +{
> +       u32 endbr;
> +
> +       __get_kernel_nofault(&endbr, val, u32, Efault);
> +       return __is_endbr(endbr);
> +
> +Efault:
> +       return false;
> +}
> +
>  static void poison_cfi(void *addr);
>
>  static void __init_or_module poison_endbr(void *addr, bool warn)
>  {
> -       u32 endbr, poison = gen_endbr_poison();
> -
> -       if (WARN_ON_ONCE(get_kernel_nofault(endbr, addr)))
> -               return;
> +       u32 poison = gen_endbr_poison();
>
> -       if (!is_endbr(endbr)) {
> +       if (!is_endbr(addr)) {
>                 WARN_ON_ONCE(warn);
>                 return;
>         }
> @@ -988,7 +996,7 @@ static u32  cfi_seed __ro_after_init;
>  static u32 cfi_rehash(u32 hash)
>  {
>         hash ^= cfi_seed;
> -       while (unlikely(is_endbr(hash) || is_endbr(-hash))) {
> +       while (unlikely(__is_endbr(hash) || __is_endbr(-hash))) {
>                 bool lsb = hash & 1;
>                 hash >>= 1;
>                 if (lsb)
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -373,16 +373,7 @@ static bool can_probe(unsigned long padd
>  kprobe_opcode_t *arch_adjust_kprobe_addr(unsigned long addr, unsigned long offset,
>                                          bool *on_func_entry)
>  {
> -       u32 insn;
> -
> -       /*
> -        * Since 'addr' is not guaranteed to be safe to access, use
> -        * copy_from_kernel_nofault() to read the instruction:
> -        */
> -       if (copy_from_kernel_nofault(&insn, (void *)addr, sizeof(u32)))
> -               return NULL;
> -
> -       if (is_endbr(insn)) {
> +       if (is_endbr((u32 *)addr)) {
>                 *on_func_entry = !offset || offset == 4;
>                 if (*on_func_entry)
>                         offset = 4;
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -625,7 +625,7 @@ int bpf_arch_text_poke(void *ip, enum bp
>          * See emit_prologue(), for IBT builds the trampoline hook is preceded
>          * with an ENDBR instruction.
>          */
> -       if (is_endbr(*(u32 *)ip))
> +       if (is_endbr(ip))
>                 ip += ENDBR_INSN_SIZE;
>
>         return __bpf_arch_text_poke(ip, t, old_addr, new_addr);
> @@ -2971,7 +2971,7 @@ static int __arch_prepare_bpf_trampoline
>                 /* skip patched call instruction and point orig_call to actual
>                  * body of the kernel function.
>                  */
> -               if (is_endbr(*(u32 *)orig_call))
> +               if (is_endbr(orig_call))
>                         orig_call += ENDBR_INSN_SIZE;
>                 orig_call += X86_PATCH_SIZE;
>         }
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1027,19 +1027,9 @@ static const struct bpf_func_proto bpf_g
>  #ifdef CONFIG_X86_KERNEL_IBT
>  static unsigned long get_entry_ip(unsigned long fentry_ip)
>  {
> -       u32 instr;
> +       if (is_endbr((void *)(fentry_ip - ENDBR_INSN_SIZE)))
> +               return fentry_ip - ENDBR_INSN_SIZE;
>
> -       /* We want to be extra safe in case entry ip is on the page edge,
> -        * but otherwise we need to avoid get_kernel_nofault()'s overhead.
> -        */
> -       if ((fentry_ip & ~PAGE_MASK) < ENDBR_INSN_SIZE) {
> -               if (get_kernel_nofault(instr, (u32 *)(fentry_ip - ENDBR_INSN_SIZE)))
> -                       return fentry_ip;
> -       } else {
> -               instr = *(u32 *)(fentry_ip - ENDBR_INSN_SIZE);
> -       }
> -       if (is_endbr(instr))
> -               fentry_ip -= ENDBR_INSN_SIZE;
>         return fentry_ip;
>  }
>  #else

LGTM.

Acked-by: Andrii Nakryiko <andrii@...nel.org>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ