| 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: <20250219194441.GA28384@noisy.programming.kicks-ass.net>
Date: Wed, 19 Feb 2025 20:44:41 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: Kees Cook <kees@...nel.org>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org, alyssa.milburn@...el.com,
scott.d.constable@...el.com, joao@...rdrivepizza.com,
andrew.cooper3@...rix.com, jpoimboe@...nel.org,
jose.marchesi@...cle.com, hjl.tools@...il.com,
ndesaulniers@...gle.com, samitolvanen@...gle.com, nathan@...nel.org,
ojeda@...nel.org, alexei.starovoitov@...il.com, mhiramat@...nel.org,
jmill@....edu
Subject: Re: [PATCH v3 06/10] x86/traps: Decode LOCK Jcc.d8 #UD
On Wed, Feb 19, 2025 at 07:33:30PM +0100, Peter Zijlstra wrote:
> On Wed, Feb 19, 2025 at 10:20:25AM -0800, Kees Cook wrote:
>
> > I realize these are misplaced chunks, but passing ud_type into the
> > handler feels like a layering violation to me. I struggled with this
> > when making recommendations for the UBSAN handler too, so I'm not sure
> > I have any better idea. It feels like there should be a way to separate
> > this logic more cleanly. The handlers are all doing very similar things:
> >
> > 1- find the address where a bad thing happened
> > 2- report about it
> > 3- whether to continue execution
> > 4- where to continue execution
> >
> > The variability happens with 1 and 4, where it depends on the instruction
> > sequences. Meh, I dunno. I can't see anything cleaner, so passing down
> > ud_type does seem best.
>
> Yeah, agreed. I couldn't get rid of relying on ud_type entirely (it was
> worse), I'll see if I can come up something.
Completely untested, but I think it should work...
diff --git a/arch/x86/include/asm/cfi.h b/arch/x86/include/asm/cfi.h
index 896cf8cf4ea7..2f6a01f098b5 100644
--- a/arch/x86/include/asm/cfi.h
+++ b/arch/x86/include/asm/cfi.h
@@ -109,7 +109,7 @@ extern bhi_thunk __bhi_args_end[];
struct pt_regs;
#ifdef CONFIG_CFI_CLANG
-enum bug_trap_type handle_cfi_failure(int ud_type, struct pt_regs *regs);
+enum bug_trap_type handle_cfi_failure(struct pt_regs *regs);
#define __bpfcall
extern u32 cfi_bpf_hash;
extern u32 cfi_bpf_subprog_hash;
@@ -133,10 +133,10 @@ extern u32 cfi_get_func_hash(void *func);
extern int cfi_get_func_arity(void *func);
#ifdef CONFIG_FINEIBT
-extern bool decode_fineibt_insn(int ud_type, struct pt_regs *regs, unsigned long *target, u32 *type);
+extern bool decode_fineibt_insn(struct pt_regs *regs, unsigned long *target, u32 *type);
#else
static inline bool
-decode_fineibt_insn(int ud_type, struct pt_regs *regs, unsigned long *target, u32 *type)
+decode_fineibt_insn(struct pt_regs *regs, unsigned long *target, u32 *type)
{
return false;
}
@@ -144,7 +144,7 @@ decode_fineibt_insn(int ud_type, struct pt_regs *regs, unsigned long *target, u3
#endif
#else
-static inline enum bug_trap_type handle_cfi_failure(int ud_type, struct pt_regs *regs)
+static inline enum bug_trap_type handle_cfi_failure(struct pt_regs *regs)
{
return BUG_TRAP_TYPE_NONE;
}
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 5bf5c0283259..d00eabf092f2 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -1631,7 +1631,7 @@ static void poison_cfi(void *addr)
* We check the preamble by checking for the ENDBR instruction relative to the
* 0xEA instruction.
*/
-static bool decode_fineibt_preamble(int ud_type, struct pt_regs *regs,
+static bool decode_fineibt_preamble(struct pt_regs *regs,
unsigned long *target, u32 *type)
{
unsigned long addr = regs->ip - fineibt_preamble_ud;
@@ -1660,7 +1660,7 @@ static bool decode_fineibt_preamble(int ud_type, struct pt_regs *regs,
/*
* regs->ip points to one of the UD2 in __bhi_args[].
*/
-static bool decode_fineibt_bhi(int ud_type, struct pt_regs *regs,
+static bool decode_fineibt_bhi(struct pt_regs *regs,
unsigned long *target, u32 *type)
{
unsigned long addr;
@@ -1696,12 +1696,15 @@ static bool decode_fineibt_bhi(int ud_type, struct pt_regs *regs,
* regs->ip points to a LOCK Jcc.d8 instruction from the fineibt_paranoid_start[]
* sequence.
*/
-static bool decode_fineibt_paranoid(int ud_type, struct pt_regs *regs,
+static bool decode_fineibt_paranoid(struct pt_regs *regs,
unsigned long *target, u32 *type)
{
unsigned long addr = regs->ip - fineibt_paranoid_ud;
u32 hash;
+ if (!is_cfi_trap(addr + fineibt_caller_size - LEN_UD2))
+ return false;
+
__get_kernel_nofault(&hash, addr + fineibt_caller_hash, u32, Efault);
*target = regs->r11 + fineibt_preamble_size;
*type = regs->r10;
@@ -1716,15 +1719,17 @@ static bool decode_fineibt_paranoid(int ud_type, struct pt_regs *regs,
return false;
}
-bool decode_fineibt_insn(int ud_type, struct pt_regs *regs,
+bool decode_fineibt_insn(struct pt_regs *regs,
unsigned long *target, u32 *type)
{
- if (ud_type == BUG_LOCK)
- return decode_fineibt_paranoid(ud_type, regs, target, type);
if (regs->ip > (unsigned long)__bhi_args &&
regs->ip < (unsigned long)__bhi_args_end)
- return decode_fineibt_bhi(ud_type, regs, target, type);
- return decode_fineibt_preamble(ud_type, regs, target, type);
+ return decode_fineibt_bhi(regs, target, type);
+
+ if (decode_fineibt_paranoid(regs, target, type))
+ return true;
+
+ return decode_fineibt_preamble(regs, target, type);
}
#else
diff --git a/arch/x86/kernel/cfi.c b/arch/x86/kernel/cfi.c
index a1b03255f4b9..77086cf565ec 100644
--- a/arch/x86/kernel/cfi.c
+++ b/arch/x86/kernel/cfi.c
@@ -65,7 +65,7 @@ static bool decode_cfi_insn(struct pt_regs *regs, unsigned long *target,
* Checks if a ud2 trap is because of a CFI failure, and handles the trap
* if needed. Returns a bug_trap_type value similarly to report_bug.
*/
-enum bug_trap_type handle_cfi_failure(int ud_type, struct pt_regs *regs)
+enum bug_trap_type handle_cfi_failure(struct pt_regs *regs)
{
unsigned long target, addr = regs->ip;
u32 type;
@@ -81,7 +81,7 @@ enum bug_trap_type handle_cfi_failure(int ud_type, struct pt_regs *regs)
break;
case CFI_FINEIBT:
- if (!decode_fineibt_insn(ud_type, regs, &target, &type))
+ if (!decode_fineibt_insn(regs, &target, &type))
return BUG_TRAP_TYPE_NONE;
break;
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 67fa578c451b..8945e3e23240 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -333,21 +333,18 @@ static noinstr bool handle_bug(struct pt_regs *regs)
raw_local_irq_enable();
switch (ud_type) {
- case BUG_EA:
- case BUG_LOCK:
- if (handle_cfi_failure(ud_type, regs) == BUG_TRAP_TYPE_WARN) {
- if (regs->ip == addr)
- regs->ip += ud_len;
+ case BUG_UD2:
+ if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN ) {
handled = true;
+ break;
}
- break;
+ fallthrough;
- case BUG_UD2:
- if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN ||
- handle_cfi_failure(ud_type, regs) == BUG_TRAP_TYPE_WARN) {
- if (regs->ip == addr)
- regs->ip += ud_len;
+ case BUG_EA:
+ case BUG_LOCK:
+ if (handle_cfi_failure(regs) == BUG_TRAP_TYPE_WARN) {
handled = true;
+ break;
}
break;
@@ -363,6 +360,10 @@ static noinstr bool handle_bug(struct pt_regs *regs)
break;
}
+ /* Continue after the trapping instruction, unless overridden. */
+ if (handled && regs->ip == addr)
+ regs->ip += ud_len;
+
/* Restore failure location if we're not continuing execution. */
if (!handled && regs->ip != addr)
regs->ip = addr;
Powered by blists - more mailing lists