Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tag accesses to stack pointer TLS locations in call gates #462

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

fw-immunant
Copy link
Contributor

We previously observed the following:

Thread 2.1 received signal SIGSEGV, Segmentation fault
Memory tag violation while accessing address 0x00007ffff7f92028
Allocation tag 0x2
Logical tag 0x0.
[Switching to Thread 1089820.1089820]
0x00007ffff57d0a6c in __wrap_recurse_dso ()
   from build-aarch64-compiler-rt/tests/recursion/librecursion_call_gates.so
(gdb) bt
#0  0x00007ffff57d0a6c in __wrap_recurse_dso ()
   from build-aarch64-compiler-rt/tests/recursion/librecursion_call_gates.so
#1  0x00007ffff7f9f944 in recurse_main ()
#2  0x00007ffff7f9f964 in fake_criterion_recursion_main ()
#3  0x00007ffff7fa153c in main ()
(gdb) disassemble 
Dump of assembler code for function __wrap_recurse_dso:
   0x00007ffff57d0a14 <+0>:	stp	x29, x30, [sp, #-16]!
   0x00007ffff57d0a18 <+4>:	mov	x29, sp
   0x00007ffff57d0a1c <+8>:	sub	sp, sp, #0x50
   0x00007ffff57d0a20 <+12>:	str	x19, [sp]
   0x00007ffff57d0a24 <+16>:	str	x20, [sp, #8]
   0x00007ffff57d0a28 <+20>:	str	x21, [sp, #16]
   0x00007ffff57d0a2c <+24>:	str	x22, [sp, #24]
   0x00007ffff57d0a30 <+28>:	str	x23, [sp, #32]
   0x00007ffff57d0a34 <+32>:	str	x24, [sp, #40]
   0x00007ffff57d0a38 <+36>:	str	x25, [sp, #48]
   0x00007ffff57d0a3c <+40>:	str	x26, [sp, #56]
   0x00007ffff57d0a40 <+44>:	str	x27, [sp, #64]
   0x00007ffff57d0a44 <+48>:	str	x28, [sp, #72]
   0x00007ffff57d0a48 <+52>:	mrs	x9, tpidr_el0
   0x00007ffff57d0a4c <+56>:	adrp	x10, 0x7ffff57ef000
   0x00007ffff57d0a50 <+60>:	ldr	x10, [x10, #4040]
   0x00007ffff57d0a54 <+64>:	add	x10, x10, x9
   0x00007ffff57d0a58 <+68>:	mov	x12, sp
   0x00007ffff57d0a5c <+72>:	str	x12, [x10]
   0x00007ffff57d0a60 <+76>:	adrp	x10, 0x7ffff57ef000
   0x00007ffff57d0a64 <+80>:	ldr	x10, [x10, #4072]
   0x00007ffff57d0a68 <+84>:	add	x10, x10, x9
=> 0x00007ffff57d0a6c <+88>:	ldr	x11, [x10]
   0x00007ffff57d0a70 <+92>:	mov	sp, x11
   0x00007ffff57d0a74 <+96>:	str	x0, [sp, #-16]!
   0x00007ffff57d0a78 <+100>:	bl	0x7ffff57d09a0 <__libia2_scrub_registers>
   0x00007ffff57d0a7c <+104>:	ldr	x0, [sp]
   0x00007ffff57d0a80 <+108>:	add	sp, sp, #0x10
   0x00007ffff57d0a84 <+112>:	mov	x18, #0x200000000000000     	// #144115188075855872
   0x00007ffff57d0a88 <+116>:	bl	0x7ffff57d07f0 <recurse_dso@plt>
   0x00007ffff57d0a8c <+120>:	mrs	x9, tpidr_el0
   0x00007ffff57d0a90 <+124>:	adrp	x10, 0x7ffff57ef000
   0x00007ffff57d0a94 <+128>:	ldr	x10, [x10, #4072]
   0x00007ffff57d0a98 <+132>:	add	x10, x10, x9
   0x00007ffff57d0a9c <+136>:	mov	x12, sp

These TLS accesses to load/save the stack pointer are untagged. I'm actually unsure how the first one of these succeeded, as it should have failed due to accessing ia2_stackptr_1 without a tagged pointer.

For reference, this is the asm source of this call gate (before this PR):

#include <ia2.h>
#include <scrub_registers.h>
void *ia2_fn_ptr;
asm(
    /* Wrapper for recurse_dso(int): */
    ".text\n"
    ".global __wrap_recurse_dso\n"
    ".type __wrap_recurse_dso, @function\n"
    "__wrap_recurse_dso:\n"
    "stp x29, x30, [sp, #-16]!\n"
    "mov x29, sp\n"
    "sub sp, sp, #80\n"
    "str x19, [sp, #0]\n"
    "str x20, [sp, #8]\n"
    "str x21, [sp, #16]\n"
    "str x22, [sp, #24]\n"
    "str x23, [sp, #32]\n"
    "str x24, [sp, #40]\n"
    "str x25, [sp, #48]\n"
    "str x26, [sp, #56]\n"
    "str x27, [sp, #64]\n"
    "str x28, [sp, #72]\n"
    /* Compute location to save old stack pointer in x10 */
    "mrs x9, tpidr_el0\n"
    "adrp x10, :gottprel:ia2_stackptr_1\n"
    "ldr x10, [x10, #:gottprel_lo12:ia2_stackptr_1]\n"
    "add x10, x10, x9\n"
    /* Write old stack pointer to memory */
    "mov x12, sp\n"
    "str x12, [x10]\n"
    /* Compute location to load new stack pointer in x10 */
    "adrp x10, :gottprel:ia2_stackptr_2\n"
    "ldr x10, [x10, #:gottprel_lo12:ia2_stackptr_2]\n"
    "add x10, x10, x9\n"
    /* Read new stack pointer from memory */
    "ldr x11, [x10]\n"
    "mov sp, x11\n"
    /* Preserve essential regs on stack */
    "str x0, [sp, #-16]!\n"
    /* Scrub non-essential regs */
    "bl __libia2_scrub_registers\n"
    /* Restore preserved regs */
    "ldr x0, [sp]\n"
    "add sp, sp, #16\n"
    "movz x18, #0x0200, LSL #48\n"
    /* Call wrapped function */
    "bl recurse_dso\n"
    /* Compute location to save old stack pointer in x10 */
    "mrs x9, tpidr_el0\n"
    "adrp x10, :gottprel:ia2_stackptr_2\n"
    "ldr x10, [x10, #:gottprel_lo12:ia2_stackptr_2]\n"
    "add x10, x10, x9\n"
    /* Write old stack pointer to memory */
    "mov x12, sp\n"
    "str x12, [x10]\n"
    /* Compute location to load new stack pointer in x10 */
    "adrp x10, :gottprel:ia2_stackptr_1\n"
    "ldr x10, [x10, #:gottprel_lo12:ia2_stackptr_1]\n"
    "add x10, x10, x9\n"
    /* Read new stack pointer from memory */
    "ldr x11, [x10]\n"
    "mov sp, x11\n"
    /* Scrub non-essential regs */
    "bl __libia2_scrub_registers\n"
    "movz x18, #0x0100, LSL #48\n"
    "ldr x19, [sp, #0]\n"
    "ldr x20, [sp, #8]\n"
    "ldr x21, [sp, #16]\n"
    "ldr x22, [sp, #24]\n"
    "ldr x23, [sp, #32]\n"
    "ldr x24, [sp, #40]\n"
    "ldr x25, [sp, #48]\n"
    "ldr x26, [sp, #56]\n"
    "ldr x27, [sp, #64]\n"
    "ldr x28, [sp, #72]\n"
    "add sp, sp, #80\n"
    "ldp x29, x30, [sp], #16\n"
    /* Return to the caller */
    "ret\n"
    ".size __wrap_recurse_dso, .-__wrap_recurse_dso\n"
    ".previous\n"
);

this fixes the recursion test on AArch64
@fw-immunant
Copy link
Contributor Author

Looks like there are still violations in the recursion test in CI. Strange that my local unpatched (QEMU) doesn't halt on them...

@ayrtonm
Copy link
Contributor

ayrtonm commented Nov 1, 2024

Aren't ia2_stackptr_ tagged pointers already? I don't see why this is necessary. One of the todos from #333 was making sure that loads/stores in the call gates use tagged pointers, but most of them were ok since they're mainly derived from stack pointers. I think this might be the only one where we couldn't guarantee the address was tagged #333 (comment)

@fw-immunant
Copy link
Contributor Author

Aren't ia2_stackptr_ tagged pointers already? I don't see why this is necessary. One of the todos from #333 was making sure that loads/stores in the call gates use tagged pointers, but most of them were ok since they're mainly derived from stack pointers. I think this might be the only one where we couldn't guarantee the address was tagged #333 (comment)

ia2_stackptr_N hold tagged pointers to the individual per-thread+compartment stacks; that tagging happened in 11dcc5a. But accesses to these thread locals must also be performed through tagged pointers, and the accesses in our wrappers were previously being done without tagging.

@fw-immunant fw-immunant requested a review from ayrtonm November 4, 2024 21:30
Copy link
Contributor

@ayrtonm ayrtonm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fw-immunant
Copy link
Contributor Author

Merging; there's no reason to leave this sitting here with r+, though we do need to look into these test failures in CI.

@fw-immunant fw-immunant merged commit 564f95e into main Dec 9, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants