On Mon, 2025-03-17 at 17:24 -0700, Josh Poimboeuf wrote:
> On Mon, Mar 17, 2025 at 05:17:24PM -0700, Josh Poimboeuf wrote:
> > On Mon, Mar 17, 2025 at 12:40:14PM +0000, David Woodhouse wrote:
> > > On Fri, 2025-03-14 at 10:52 -0700, Josh Poimboeuf wrote:
> > > >
> > > > IIRC, the reasons were the patched alternative, and also you
> > > > wanted to
> > > > disassemble (but note that's still possible with gdb).
> > >
> > > It's meaningful output from 'objdump -S' that I miss. But OK.
> >
> > FYI, this works:
> >
> > objdump -Srw -j .data..relocate_kernel vmlinux.o
>
> ... but I see now that it doesn't intersperse the source. Never
> mind...
To be fair, the source is assembler. So it isn't *so* hard to work it
out.
But on the whole, I'm not sure the CFI check is worth it.
CFI checks that the caller and callee agree about the prototype of the
function being called. There are two main benefits of this:
• to protect against attacks where function pointers are substituted
for gadgets.
• to protect against genuine bugs, where the caller and the callee
disagree about the function arguments.
For the relocate_kernel() case I don't think we care much about the
first. Without a CFI prologue, no *other* code can be tricked into
calling relocate_kernel() — and besides, it's in the kernel's data
section and isn't executable anyway until the kexec code copies it to a
page that *is*. And the kexec code itself isn't just following an
arbitrary function pointer; it copies the code into the control page
and invokes it there, so it's unlikely to follow a bogus pointer
either.
It's the *second* benefit which is more relevant to us, ensuring that
the caller and the callee genuinely do agree about the prototype.
But using the SYM_TYPED_FUNC_START() macro doesn't give us that; the
CFI prologue of the asm function is just using the signature emitted by
the *caller* in the weak __kcfi_typeid_relocate_kernel symbol anyway,
so they're never going to disagree. And how *could* the assembler side
build a typeid signature from the asm anyway?
I suspect that just leaving it marked __nocfi is probably the easier
option, unless I can *hardcode* the function signature 0x19835999 in
the CFI prologue in relocate_kernel_64.S to protect against someone
accidentally changing the C-visible 'relocate_kernel_fn' typedef
without changing the corresponding assembler? But honestly, is that
likely?
Looks like this (with your objtool relaxation on top, as well as
removing load_segments() in machine_kexec_64.c if I want the check to
actually emit the warning correctly.).
I just don't think it's worth it...
From 1bbed9c611fd286b68e2c459320910c4fefd4a22 Mon Sep 17 00:00:00 2001
From: David Woodhouse <dwmw@xxxxxxxxxxxx>
Date: Mon, 16 Dec 2024 10:26:24 +0000
Subject: [PATCH] x86/kexec: Add CFI type information to relocate_kernel()
A previous commit added __nocfi to machine_kexec() because it makes an
indirect call to relocate_kernel() which lacked CFI type information,
and caused the system to crash.
Use SYM_TYPED_FUNC_START() to ensure that the type information is
present, and remove the __nocfi tag. And emit the *actual* type
signature (0x19835999) because by default, asm code uses the type
signature emitted by the *caller*, which is never going to differ!
Signed-off-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
---
arch/x86/kernel/machine_kexec_64.c | 2 +-
arch/x86/kernel/relocate_kernel_64.S | 19 ++++++++++++++++++-
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
index 016862d2b544..e1f5fc858aee 100644
--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -396,7 +396,7 @@ void machine_kexec_cleanup(struct kimage *image)
* Do not allocate memory (or fail in any way) in machine_kexec().
* We are past the point of no return, committed to rebooting now.
*/
-void __nocfi machine_kexec(struct kimage *image)
+void machine_kexec(struct kimage *image)
{
unsigned long reloc_start = (unsigned long)__relocate_kernel_start;
relocate_kernel_fn *relocate_kernel_ptr;
diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
index 814af7fa6318..428401e55d29 100644
--- a/arch/x86/kernel/relocate_kernel_64.S
+++ b/arch/x86/kernel/relocate_kernel_64.S
@@ -5,6 +5,7 @@
*/
#include <linux/linkage.h>
+#include <linux/cfi_types.h>
#include <linux/stringify.h>
#include <asm/alternative.h>
#include <asm/page_types.h>
@@ -67,8 +68,24 @@ SYM_DATA_END(kexec_debug_idt)
* a data section even of the object file, to prevent objtool from having
* opinions about it.
*/
+
+#ifdef CONFIG_CFI_CLANG
+/*
+ * The SYM_TYPED_FUNC_START macro emits a CFI prologue for the function,
+ * referencing the __kcfi_typeid_##name symbol which is the signature of
+ * the function prototype. The value of that symbol ends up coming from
+ * a weak symbol emitted by the *caller* (in this case machine_kexec()),
+ * which means it's *entirely* useless for checking that the caller and
+ * callee agree about the prototype of the (asm) function being called.
+ * So, we define the signature *here* for ourselves, and if the C code
+ * ever calls relocate_kernel() in the belief that it has a different
+ * prototype, then the CFI check will trigger as $DEITY intended.
+ */
+ .weak __kcfi_typeid_relocate_kernel
+ .set __kcfi_typeid_relocate_kernel, 0x19835999
+#endif
.code64
-SYM_CODE_START_NOALIGN(relocate_kernel)
+SYM_TYPED_FUNC_START(relocate_kernel)
/*
* %rdi indirection_page
* %rsi pa_control_page
--
2.48.1
<<attachment: smime.p7s>>
