| 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
| ||
|
Message-ID: <20250206123307.GO7145@noisy.programming.kicks-ass.net>
Date: Thu, 6 Feb 2025 13:33:07 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: Masami Hiramatsu <mhiramat@...nel.org>
Cc: Steven Rostedt <rostedt@...dmis.org>,
Gabriel de Perthuis <g2p.code@...il.com>,
Haiyue Wang <haiyuewa@....com>,
Sami Tolvanen <samitolvanen@...gle.com>,
Mark Rutland <mark.rutland@....com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>,
"H . Peter Anvin" <hpa@...or.com>, x86@...nel.org,
linux-trace-kernel@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ftrace: x86: Fix a compile error about
get_kernel_nofault()
On Thu, Feb 06, 2025 at 01:13:28PM +0100, Peter Zijlstra wrote:
> No, it just cleans up this utter trainwreck. I also noticed this is a
> second (new) copy of this garbage. Clearly I didn't yell loud enough
> last time and people didn't think to vomit when doing the copy/paste :/
>
> Function will look something like:
>
> unsigned long arch_ftrace_get_symaddr(unsigned long fentry_ip)
> {
> if (is_endbr(fentry_ip - ENDBR_INSN_SIZE))
> fentry_op -= ENDBR_INSN_SIZE;
> return fentry_ip;
> }
>
> Let me finish local build test before I push out.
Bah, still waiting for a LLVM build, but patch should be here:
git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/ibt
I was supposed to merge this last cycle, but then akpm shat all over
arch/x86/kernel/alternative.c and we had to clean that up :/
Notably, this patch, not sure it applies out of order.
---
Subject: x86/ibt: Clean up is_endbr()
From: Peter Zijlstra <peterz@...radead.org>
Date: Mon Nov 27 09:58:06 CET 2023
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>
Acked-by: Alexei Starovoitov <ast@...nel.org>
Acked-by: Andrii Nakryiko <andrii@...nel.org>
---
arch/x86/events/core.c | 2 +-
arch/x86/include/asm/ftrace.h | 17 +++--------------
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 | 20 +++-----------------
7 files changed, 27 insertions(+), 52 deletions(-)
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -2857,7 +2857,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/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -2,6 +2,7 @@
#ifndef _ASM_X86_FTRACE_H
#define _ASM_X86_FTRACE_H
+#include "asm/ibt.h"
#include <asm/ptrace.h>
#ifdef CONFIG_FUNCTION_TRACER
@@ -36,21 +37,9 @@ static inline unsigned long ftrace_call_
static inline unsigned long arch_ftrace_get_symaddr(unsigned long fentry_ip)
{
-#ifdef CONFIG_X86_KERNEL_IBT
- u32 instr;
+ if (is_endbr((void*)(fentry_ip - ENDBR_INSN_SIZE)))
+ fentry_ip -= ENDBENDBR_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;
-#endif
return fentry_ip;
}
#define ftrace_get_symaddr(fentry_ip) arch_ftrace_get_symaddr(fentry_ip)
--- 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);
@@ -98,7 +99,7 @@ extern __noendbr void ibt_restore(u64 sa
#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;
}
@@ -984,7 +992,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
@@ -641,7 +641,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);
@@ -3036,7 +3036,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
@@ -1038,27 +1038,13 @@ static const struct bpf_func_proto bpf_g
.arg1_type = ARG_PTR_TO_CTX,
};
-#ifdef CONFIG_X86_KERNEL_IBT
-static unsigned long get_entry_ip(unsigned long fentry_ip)
+static inline 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);
- }
- if (is_endbr(instr))
+ if (is_endbr((void *)(fentry_ip - ENDBR_INSN_SIZE)))
fentry_ip -= ENDBR_INSN_SIZE;
+
return fentry_ip;
}
-#else
-#define get_entry_ip(fentry_ip) fentry_ip
-#endif
BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs)
{
Powered by blists - more mailing lists