Because overlapping code sequences are all the rage.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/include/asm/bug.h | 2 ++
arch/x86/kernel/traps.c | 30 +++++++++++++++++++++++++-----
2 files changed, 27 insertions(+), 5 deletions(-)
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -17,6 +17,7 @@
* In clang we have UD1s reporting UBSAN failures on X86, 64 and 32bit.
*/
#define INSN_ASOP 0x67
+#define INSN_LOCK 0xf0
#define OPCODE_ESCAPE 0x0f
#define SECOND_BYTE_OPCODE_UD1 0xb9
#define SECOND_BYTE_OPCODE_UD2 0x0b
@@ -26,6 +27,7 @@
#define BUG_UD1 0xfffd
#define BUG_UD1_UBSAN 0xfffc
#define BUG_EA 0xffea
+#define BUG_LOCK 0xfff0
#ifdef CONFIG_GENERIC_BUG
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -97,6 +97,7 @@ __always_inline int is_valid_bugaddr(uns
* If it's a UD1, further decode to determine its use:
*
* FineIBT: ea (bad)
+ * FineIBT: 0f 75 f9 lock jne . - 6
* UBSan{0}: 67 0f b9 00 ud1 (%eax),%eax
* UBSan{10}: 67 0f b9 40 10 ud1 0x10(%eax),%eax
* static_call: 0f b9 cc ud1 %esp,%ecx
@@ -106,6 +107,7 @@ __always_inline int is_valid_bugaddr(uns
__always_inline int decode_bug(unsigned long addr, s32 *imm, int *len)
{
unsigned long start = addr;
+ bool lock = false;
u8 v;
if (addr < TASK_SIZE_MAX)
@@ -114,12 +116,29 @@ __always_inline int decode_bug(unsigned
v = *(u8 *)(addr++);
if (v == INSN_ASOP)
v = *(u8 *)(addr++);
- if (v == 0xea) {
+
+ if (v == INSN_LOCK) {
+ lock = true;
+ v = *(u8 *)(addr++);
+ }
+
+ switch (v) {
+ case 0x70 ... 0x7f: /* Jcc.d8 */
+ addr += 1; /* d8 */
+ *len = addr - start;
+ WARN_ON_ONCE(!lock);
+ return BUG_LOCK;
+
+ case 0xea:
*len = addr - start;
return BUG_EA;
- }
- if (v != OPCODE_ESCAPE)
+
+ case OPCODE_ESCAPE:
+ break;
+
+ default:
return BUG_NONE;
+ }
v = *(u8 *)(addr++);
if (v == SECOND_BYTE_OPCODE_UD2) {
@@ -315,7 +334,8 @@ static noinstr bool handle_bug(struct pt
switch (ud_type) {
case BUG_EA:
- if (handle_cfi_failure(regs) == BUG_TRAP_TYPE_WARN) {
+ case BUG_LOCK:
+ if (handle_cfi_failure(ud_type, regs) == BUG_TRAP_TYPE_WARN) {
if (regs->ip == addr)
regs->ip += ud_len;
handled = true;
@@ -324,7 +344,7 @@ static noinstr bool handle_bug(struct pt
case BUG_UD2:
if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN ||
- handle_cfi_failure(regs) == BUG_TRAP_TYPE_WARN) {
+ handle_cfi_failure(ud_type, regs) == BUG_TRAP_TYPE_WARN) {
if (regs->ip == addr)
regs->ip += ud_len;
handled = true;On Wed, Feb 19, 2025 at 05:21:13PM +0100, Peter Zijlstra wrote:
> Because overlapping code sequences are all the rage.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Kees Cook <kees@kernel.org>
Semi-pointless stream of consciousness below...
> ---
> arch/x86/include/asm/bug.h | 2 ++
> arch/x86/kernel/traps.c | 30 +++++++++++++++++++++++++-----
> 2 files changed, 27 insertions(+), 5 deletions(-)
>
> --- a/arch/x86/include/asm/bug.h
> +++ b/arch/x86/include/asm/bug.h
> @@ -17,6 +17,7 @@
> * In clang we have UD1s reporting UBSAN failures on X86, 64 and 32bit.
> */
> #define INSN_ASOP 0x67
> +#define INSN_LOCK 0xf0
> #define OPCODE_ESCAPE 0x0f
> #define SECOND_BYTE_OPCODE_UD1 0xb9
> #define SECOND_BYTE_OPCODE_UD2 0x0b
> @@ -26,6 +27,7 @@
> #define BUG_UD1 0xfffd
> #define BUG_UD1_UBSAN 0xfffc
> #define BUG_EA 0xffea
> +#define BUG_LOCK 0xfff0
>
> #ifdef CONFIG_GENERIC_BUG
>
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -97,6 +97,7 @@ __always_inline int is_valid_bugaddr(uns
> * If it's a UD1, further decode to determine its use:
> *
> * FineIBT: ea (bad)
> + * FineIBT: 0f 75 f9 lock jne . - 6
> * UBSan{0}: 67 0f b9 00 ud1 (%eax),%eax
> * UBSan{10}: 67 0f b9 40 10 ud1 0x10(%eax),%eax
> * static_call: 0f b9 cc ud1 %esp,%ecx
> @@ -106,6 +107,7 @@ __always_inline int is_valid_bugaddr(uns
> __always_inline int decode_bug(unsigned long addr, s32 *imm, int *len)
> {
> unsigned long start = addr;
> + bool lock = false;
> u8 v;
>
> if (addr < TASK_SIZE_MAX)
> @@ -114,12 +116,29 @@ __always_inline int decode_bug(unsigned
> v = *(u8 *)(addr++);
> if (v == INSN_ASOP)
> v = *(u8 *)(addr++);
> - if (v == 0xea) {
> +
> + if (v == INSN_LOCK) {
> + lock = true;
> + v = *(u8 *)(addr++);
> + }
> +
> + switch (v) {
> + case 0x70 ... 0x7f: /* Jcc.d8 */
> + addr += 1; /* d8 */
> + *len = addr - start;
> + WARN_ON_ONCE(!lock);
> + return BUG_LOCK;
> +
> + case 0xea:
> *len = addr - start;
> return BUG_EA;
> - }
> - if (v != OPCODE_ESCAPE)
> +
> + case OPCODE_ESCAPE:
> + break;
> +
> + default:
> return BUG_NONE;
> + }
>
> v = *(u8 *)(addr++);
> if (v == SECOND_BYTE_OPCODE_UD2) {
> @@ -315,7 +334,8 @@ static noinstr bool handle_bug(struct pt
>
> switch (ud_type) {
> case BUG_EA:
> - if (handle_cfi_failure(regs) == BUG_TRAP_TYPE_WARN) {
> + case BUG_LOCK:
> + if (handle_cfi_failure(ud_type, regs) == BUG_TRAP_TYPE_WARN) {
> if (regs->ip == addr)
> regs->ip += ud_len;
> handled = true;
> @@ -324,7 +344,7 @@ static noinstr bool handle_bug(struct pt
>
> case BUG_UD2:
> if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN ||
> - handle_cfi_failure(regs) == BUG_TRAP_TYPE_WARN) {
> + handle_cfi_failure(ud_type, regs) == BUG_TRAP_TYPE_WARN) {
> if (regs->ip == addr)
> regs->ip += ud_len;
> handled = true;
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.
--
Kees Cook
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.
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;
On Wed, Feb 19, 2025 at 05:21:13PM +0100, Peter Zijlstra wrote:
> @@ -315,7 +334,8 @@ static noinstr bool handle_bug(struct pt
>
> switch (ud_type) {
> case BUG_EA:
> - if (handle_cfi_failure(regs) == BUG_TRAP_TYPE_WARN) {
> + case BUG_LOCK:
> + if (handle_cfi_failure(ud_type, regs) == BUG_TRAP_TYPE_WARN) {
> if (regs->ip == addr)
> regs->ip += ud_len;
> handled = true;
> @@ -324,7 +344,7 @@ static noinstr bool handle_bug(struct pt
>
> case BUG_UD2:
> if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN ||
> - handle_cfi_failure(regs) == BUG_TRAP_TYPE_WARN) {
> + handle_cfi_failure(ud_type, regs) == BUG_TRAP_TYPE_WARN) {
> if (regs->ip == addr)
> regs->ip += ud_len;
> handled = true;
>
Damn, that ud_type change belongs to the next patch. Consider it fixed.
© 2016 - 2025 Red Hat, Inc.