Kernel entry points should be having ENDBR on for IBT configs.
The SYSCALL entry points are found through taking their respective
address in order to program them in the MSRs, while the exception
entry points are found through UNWIND_HINT_IRET_REGS.
The rule is that any UNWIND_HINT_IRET_REGS at sym+0 should have an
ENDBR, see the later objtool ibt validation patch.
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
arch/x86/entry/entry_64.S | 6 ++++++
arch/x86/entry/entry_64_compat.S | 3 +++
arch/x86/include/asm/idtentry.h | 20 +++++++++++---------
arch/x86/include/asm/segment.h | 2 +-
arch/x86/kernel/head_64.S | 5 ++++-
arch/x86/kernel/idt.c | 5 +++--
6 files changed, 28 insertions(+), 13 deletions(-)
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -86,6 +86,7 @@
SYM_CODE_START(entry_SYSCALL_64)
UNWIND_HINT_EMPTY
+ ENDBR
swapgs
/* tss.sp2 is scratch space. */
@@ -350,6 +351,7 @@ SYM_CODE_END(ret_from_fork)
.macro idtentry vector asmsym cfunc has_error_code:req
SYM_CODE_START(\asmsym)
UNWIND_HINT_IRET_REGS offset=\has_error_code*8
+ ENDBR
ASM_CLAC
.if \has_error_code == 0
@@ -417,6 +419,7 @@ SYM_CODE_END(\asmsym)
.macro idtentry_mce_db vector asmsym cfunc
SYM_CODE_START(\asmsym)
UNWIND_HINT_IRET_REGS
+ ENDBR
ASM_CLAC
pushq $-1 /* ORIG_RAX: no syscall to restart */
@@ -472,6 +475,7 @@ SYM_CODE_END(\asmsym)
.macro idtentry_vc vector asmsym cfunc
SYM_CODE_START(\asmsym)
UNWIND_HINT_IRET_REGS
+ ENDBR
ASM_CLAC
/*
@@ -533,6 +537,7 @@ SYM_CODE_END(\asmsym)
.macro idtentry_df vector asmsym cfunc
SYM_CODE_START(\asmsym)
UNWIND_HINT_IRET_REGS offset=8
+ ENDBR
ASM_CLAC
/* paranoid_entry returns GS information for paranoid_exit in EBX. */
@@ -1063,6 +1068,7 @@ SYM_CODE_END(error_return)
*/
SYM_CODE_START(asm_exc_nmi)
UNWIND_HINT_IRET_REGS
+ ENDBR
/*
* We allow breakpoints in NMIs. If a breakpoint occurs, then
--- a/arch/x86/entry/entry_64_compat.S
+++ b/arch/x86/entry/entry_64_compat.S
@@ -48,6 +48,7 @@
*/
SYM_CODE_START(entry_SYSENTER_compat)
UNWIND_HINT_EMPTY
+ ENDBR
/* Interrupts are off on entry. */
SWAPGS
@@ -198,6 +199,7 @@ SYM_CODE_END(entry_SYSENTER_compat)
*/
SYM_CODE_START(entry_SYSCALL_compat)
UNWIND_HINT_EMPTY
+ ENDBR
/* Interrupts are off on entry. */
swapgs
@@ -340,6 +342,7 @@ SYM_CODE_END(entry_SYSCALL_compat)
*/
SYM_CODE_START(entry_INT80_compat)
UNWIND_HINT_EMPTY
+ ENDBR
/*
* Interrupts are off on entry.
*/
--- a/arch/x86/include/asm/idtentry.h
+++ b/arch/x86/include/asm/idtentry.h
@@ -5,6 +5,8 @@
/* Interrupts/Exceptions */
#include <asm/trapnr.h>
+#define IDT_ALIGN (8 * (1 + HAS_KERNEL_IBT))
+
#ifndef __ASSEMBLY__
#include <linux/entry-common.h>
#include <linux/hardirq.h>
@@ -480,7 +482,7 @@ __visible noinstr void func(struct pt_re
/*
* ASM code to emit the common vector entry stubs where each stub is
- * packed into 8 bytes.
+ * packed into IDT_ALIGN bytes.
*
* Note, that the 'pushq imm8' is emitted via '.byte 0x6a, vector' because
* GCC treats the local vector variable as unsigned int and would expand
@@ -492,33 +494,33 @@ __visible noinstr void func(struct pt_re
* point is to mask off the bits above bit 7 because the push is sign
* extending.
*/
- .align 8
+ .align IDT_ALIGN
SYM_CODE_START(irq_entries_start)
vector=FIRST_EXTERNAL_VECTOR
.rept NR_EXTERNAL_VECTORS
UNWIND_HINT_IRET_REGS
0 :
+ ENDBR
.byte 0x6a, vector
jmp asm_common_interrupt
- nop
- /* Ensure that the above is 8 bytes max */
- . = 0b + 8
+ /* Ensure that the above is IDT_ALIGN bytes max */
+ .fill 0b + IDT_ALIGN - ., 1, 0x90
vector = vector+1
.endr
SYM_CODE_END(irq_entries_start)
#ifdef CONFIG_X86_LOCAL_APIC
- .align 8
+ .align IDT_ALIGN
SYM_CODE_START(spurious_entries_start)
vector=FIRST_SYSTEM_VECTOR
.rept NR_SYSTEM_VECTORS
UNWIND_HINT_IRET_REGS
0 :
+ ENDBR
.byte 0x6a, vector
jmp asm_spurious_interrupt
- nop
- /* Ensure that the above is 8 bytes max */
- . = 0b + 8
+ /* Ensure that the above is IDT_ALIGN bytes max */
+ .fill 0b + IDT_ALIGN - ., 1, 0x90
vector = vector+1
.endr
SYM_CODE_END(spurious_entries_start)
--- a/arch/x86/include/asm/segment.h
+++ b/arch/x86/include/asm/segment.h
@@ -275,7 +275,7 @@ static inline void vdso_read_cpunode(uns
* vector has no error code (two bytes), a 'push $vector_number' (two
* bytes), and a jump to the common entry code (up to five bytes).
*/
-#define EARLY_IDT_HANDLER_SIZE 9
+#define EARLY_IDT_HANDLER_SIZE (9 + 4*HAS_KERNEL_IBT)
/*
* xen_early_idt_handler_array is for Xen pv guests: for each entry in
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -372,9 +372,11 @@ SYM_CODE_START(early_idt_handler_array)
.rept NUM_EXCEPTION_VECTORS
.if ((EXCEPTION_ERRCODE_MASK >> i) & 1) == 0
UNWIND_HINT_IRET_REGS
+ ENDBR
pushq $0 # Dummy error code, to make stack frame uniform
.else
UNWIND_HINT_IRET_REGS offset=8
+ ENDBR
.endif
pushq $i # 72(%rsp) Vector number
jmp early_idt_handler_common
@@ -382,10 +384,11 @@ SYM_CODE_START(early_idt_handler_array)
i = i + 1
.fill early_idt_handler_array + i*EARLY_IDT_HANDLER_SIZE - ., 1, 0xcc
.endr
- UNWIND_HINT_IRET_REGS offset=16
SYM_CODE_END(early_idt_handler_array)
SYM_CODE_START_LOCAL(early_idt_handler_common)
+ UNWIND_HINT_IRET_REGS offset=16
+ ANNOTATE_NOENDBR
/*
* The stack is the hardware frame, an error code or zero, and the
* vector number.
--- a/arch/x86/kernel/idt.c
+++ b/arch/x86/kernel/idt.c
@@ -10,6 +10,7 @@
#include <asm/proto.h>
#include <asm/desc.h>
#include <asm/hw_irq.h>
+#include <asm/idtentry.h>
#define DPL0 0x0
#define DPL3 0x3
@@ -272,7 +273,7 @@ void __init idt_setup_apic_and_irq_gates
idt_setup_from_table(idt_table, apic_idts, ARRAY_SIZE(apic_idts), true);
for_each_clear_bit_from(i, system_vectors, FIRST_SYSTEM_VECTOR) {
- entry = irq_entries_start + 8 * (i - FIRST_EXTERNAL_VECTOR);
+ entry = irq_entries_start + IDT_ALIGN * (i - FIRST_EXTERNAL_VECTOR);
set_intr_gate(i, entry);
}
@@ -283,7 +284,7 @@ void __init idt_setup_apic_and_irq_gates
* system_vectors bitmap. Otherwise they show up in
* /proc/interrupts.
*/
- entry = spurious_entries_start + 8 * (i - FIRST_SYSTEM_VECTOR);
+ entry = spurious_entries_start + IDT_ALIGN * (i - FIRST_SYSTEM_VECTOR);
set_intr_gate(i, entry);
}
#endif
On Thu, Feb 24, 2022 at 03:51:45PM +0100, Peter Zijlstra wrote:
> The SYSCALL entry points are found through taking their respective
> address in order to program them in the MSRs, while the exception
> entry points are found through UNWIND_HINT_IRET_REGS.
Stupid question: does CET consider exception and syscall entry points to
be indirect calls? (I would expect so, but they're ever so slightly
differently executed...)
> [...]
> 0 :
> + ENDBR
> .byte 0x6a, vector
> jmp asm_common_interrupt
> - nop
> - /* Ensure that the above is 8 bytes max */
> - . = 0b + 8
> + /* Ensure that the above is IDT_ALIGN bytes max */
> + .fill 0b + IDT_ALIGN - ., 1, 0x90
IIUC, these are just padding -- let's use 0xcc instead of 0x90 as we do
in other places (e.g. vmlinux.lds.S).
--
Kees Cook
On Thu, Feb 24, 2022 at 03:51:45PM +0100, Peter Zijlstra wrote:
> Kernel entry points should be having ENDBR on for IBT configs.
>
> The SYSCALL entry points are found through taking their respective
> address in order to program them in the MSRs, while the exception
> entry points are found through UNWIND_HINT_IRET_REGS.
>
> The rule is that any UNWIND_HINT_IRET_REGS at sym+0 should have an
> ENDBR, see the later objtool ibt validation patch.
Could the "rule" be changed to only check global syms? It seems
unlikely a local symbol would need ENDBR.
Then you wouldn't need this annotation:
> SYM_CODE_START_LOCAL(early_idt_handler_common)
> + UNWIND_HINT_IRET_REGS offset=16
> + ANNOTATE_NOENDBR
> /*
> * The stack is the hardware frame, an error code or zero, and the
> * vector number.
--
Josh