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

Fix stack tagging on AArch64 #407

Merged
merged 4 commits into from
Oct 31, 2024
Merged

Fix stack tagging on AArch64 #407

merged 4 commits into from
Oct 31, 2024

Conversation

fw-immunant
Copy link
Contributor

@fw-immunant fw-immunant commented Oct 3, 2024

We weren't tagging the entire requested region but only its first page (which also impacted protection of non-stack pages). (Fixed separately.)

After allocating and tagging a fresh compartment stack, we returned an untagged pointer to it, which led to MTE violations. I was surprised by this because the violations I observed seemed to come from sp-relative accesses, which I thought would not perform MTE checks, but they did anyway--it would be good to write a test program to characterize exactly which instructions do and don't perform MTE checks.

In addition, we need to apply tags for the access to TLS when accessing ia2_stackptr_N.

@@ -26,6 +26,10 @@ char *allocate_stack(int i) {
exit(-1);
}
}
#ifdef __aarch64__
/* Tag the allocated stack pointer so it is accessed with the right pkey */
stack = (char *)((uint64_t)stack | (uint64_t)i << 56);
Copy link
Contributor

Choose a reason for hiding this comment

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

are we sure stack's upper bits are initially zero or should we explicitly clear them out first?

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.

I had one question but otherwise LGTM. Thanks for catching this oversight.

@ayrtonm
Copy link
Contributor

ayrtonm commented Oct 3, 2024

the violations I observed seemed to come from sp-relative accesses, which I thought would not perform MTE checks

This is expected for some instructions using sp. I'll find a link in a sec

@ayrtonm
Copy link
Contributor

ayrtonm commented Oct 3, 2024

Here's what I thought was a bug in QEMU which pointed out that some sp-relative accesses are checked. https://gitlab.com/qemu-project/qemu/-/issues/514. I think it's basically those that access "sp + runtime offset" since that can't be known to access the right thing statically. For example, see tag_checked in Shared Decode for STP.

@fw-immunant
Copy link
Contributor Author

CI here is failing for a really interesting reason: passing tagged pointers into the stack as system call arguments returns EFAULT (bad address)...

@ayrtonm
Copy link
Contributor

ayrtonm commented Oct 4, 2024

passing tagged pointers into the stack as system call arguments returns EFAULT (bad address)...

our mprotect or syscalls in general?

@ayrtonm
Copy link
Contributor

ayrtonm commented Oct 4, 2024

oh I'm guessing it probably needs the prctl stuff from #334.

@fw-immunant
Copy link
Contributor Author

oh I'm guessing it probably needs the prctl stuff from #334.

Ah, yep--that's why it was working when I wrote these commits but not when I pulled them out in to their own PR.

note that this commit does regress simple1, but I believe it does so by unmasking a hidden failure: nonzero return values from the wrapped `main` were getting lost as we clobbered `x0`
@fw-immunant
Copy link
Contributor Author

I confirmed visually that only the 4 ARM tests we expect to fail are producing MTE violations in CI:

  • recursion
  • read_config
  • simple1
  • static_addr_taken

Next we should make these failures act as test failures in CI and also actually fix them.

@fw-immunant fw-immunant merged commit 322e9db into main Oct 31, 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