| Message ID | 20240925150059.3955569-44-ardb+git@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86: Rely on toolchain for relocatable code | expand |
On Wed, 25 Sept 2024 at 08:16, Ard Biesheuvel <ardb+git@google.com> 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
On Wed, 25 Sept 2024 at 18:39, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Wed, 25 Sept 2024 at 08:16, Ard Biesheuvel <ardb+git@google.com> 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.
On 25/09/2024 5:45 pm, Ard Biesheuvel wrote: > On Wed, 25 Sept 2024 at 18:39, Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> 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. But does that work? Won't that will push the 8 bytes from the start of arch_rethook_trampoline, when what's wanted is simply the address of arch_rethook_trampoline itself. ~Andrew
On 25.09.2024 18:51, Andrew Cooper wrote: > On 25/09/2024 5:45 pm, Ard Biesheuvel wrote: >> On Wed, 25 Sept 2024 at 18:39, Linus Torvalds >> <torvalds@linux-foundation.org> wrote: >>> 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. > > But does that work? Won't that will push the 8 bytes from the start of > arch_rethook_trampoline, when what's wanted is simply the address of > arch_rethook_trampoline itself. What you describe is pushq arch_rethook_trampoline(%rip) The @GOTPCREL makes the PUSH access an item from the GOT, and that item is arch_rethook_trampoline's address. Jan
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. */