[RFC,14/28] x86/rethook: Use RIP-relative reference for return address

Commit Message

Ard Biesheuvel Sept. 25, 2024, 3:01 p.m. UTC

From: Ard Biesheuvel <[e-mail]>

Instead of pushing an immediate absolute address, which is incompatible
with PIE codegen or linking, use a LEA instruction to take the address
into a register.

Signed-off-by: Ard Biesheuvel <[e-mail]>
---
 arch/x86/kernel/rethook.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Linus Torvalds Sept. 25, 2024, 4:39 p.m. UTC | #1

On Wed, 25 Sept 2024 at 08:16, Ard Biesheuvel <[e-mail]> wrote:
>
> Instead of pushing an immediate absolute address, which is incompatible
> with PIE codegen or linking, use a LEA instruction to take the address
> into a register.

I don't think you can do this - it corrupts %rdi.

Yes, the code uses  %rdi later, but that's inside the SAVE_REGS_STRING
/ RESTORE_REGS_STRING area.

And we do have special calling conventions that aren't the regular
ones, so %rdi might actually be used elsewhere. For example,
__get_user_X and __put_user_X all have magical calling conventions:
they don't actually use %rdi, but part of the calling convention is
that the unused registers aren't modified.

Of course, I'm not actually sure you can probe those and trigger this
issue, but it all makes me think it's broken.

And it's entirely possible that I'm wrong for some reason, but this
just _looks_ very very wrong to me.

I think you can do this with a "pushq mem" instead, and put the
relocation into the memory location.

                 Linus

Ard Biesheuvel Sept. 25, 2024, 4:45 p.m. UTC | #2

On Wed, 25 Sept 2024 at 18:39, Linus Torvalds
<[e-mail]> wrote:
>
> On Wed, 25 Sept 2024 at 08:16, Ard Biesheuvel <[e-mail]> wrote:
> >
> > Instead of pushing an immediate absolute address, which is incompatible
> > with PIE codegen or linking, use a LEA instruction to take the address
> > into a register.
>
> I don't think you can do this - it corrupts %rdi.
>
> Yes, the code uses  %rdi later, but that's inside the SAVE_REGS_STRING
> / RESTORE_REGS_STRING area.
>

Oops, I missed that.

> And we do have special calling conventions that aren't the regular
> ones, so %rdi might actually be used elsewhere. For example,
> __get_user_X and __put_user_X all have magical calling conventions:
> they don't actually use %rdi, but part of the calling convention is
> that the unused registers aren't modified.
>
> Of course, I'm not actually sure you can probe those and trigger this
> issue, but it all makes me think it's broken.
>
> And it's entirely possible that I'm wrong for some reason, but this
> just _looks_ very very wrong to me.
>
> I think you can do this with a "pushq mem" instead, and put the
> relocation into the memory location.
>

I'll change this into

  pushq arch_rethook_trampoline@GOTPCREL(%rip)

which I had originally. I was trying to avoid the load from memory,
but that obviously only works if the register is not live.

Patch

diff --git a/arch/x86/kernel/rethook.c b/arch/x86/kernel/rethook.c
index 8a1c0111ae79..3b3c17ba3cd5 100644
--- a/arch/x86/kernel/rethook.c
+++ b/arch/x86/kernel/rethook.c
@@ -27,7 +27,8 @@  asm(
 #ifdef CONFIG_X86_64
 	ANNOTATE_NOENDBR	/* This is only jumped from ret instruction */
 	/* Push a fake return address to tell the unwinder it's a rethook. */
-	"	pushq $arch_rethook_trampoline\n"
+	"	leaq arch_rethook_trampoline(%rip), %rdi\n"
+	"	pushq %rdi\n"
 	UNWIND_HINT_FUNC
 	"       pushq $" __stringify(__KERNEL_DS) "\n"
 	/* Save the 'sp - 16', this will be fixed later. */