Skip to content

[ASan]Fix logic bugs that break RestrictMemoryToMaxAddress (#124712) #9991

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

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

thetruestblue
Copy link

There are two logic bugs breaking RestrictMemoryToMaxAddress.

  1. adding left_padding within MapDynamicShadow.
  • RoundUpTo((uptr)free_begin + left_padding, alignment) already adjusts for left padding. Adding this additionally within MapDynamicShadow causes us to allocate a page larger than necessary.
  • This incorrect calculation also means RestrictMemoryToMaxAddress will never find a big enough gap.
  1. There is also an issue with the expectation of hitting KERN_INVALID_ADDRESS when we are beyond the addressable regions.
  • For most embedded scenarios, we exceed vm_max_address without getting KREN_INVALID_ADDRESS so we setting max_occupied_address to a memory region the process doesn't have access to, beyond the max address, and that space is never marked as available so we never find a valid gap in those regions.
  • At some point previous it seems the assumption was once we were beyond the Max address we could expect KREN_INVALID_ADDRESS, which is no longer true up through the extended space not given to most processes.
  • Because of this, the check if (new_max_vm < max_occupied_addr) will always fail and we will never restrict the address on smaller devices.
  • Additionally because of the extra page added by adding left_padding, and how we only minimally restrict the vm, there's a chance we restrict the vm only enough for the correctly calculated size of shadow. In these cases, restricting the vm max address and will always fail due to the extra page added to space size.

credit to @delcypher for the left_padding diagnosis, remembered his old radar and PR when investigating this. https://reviews.llvm.org/D85389

Will monitor closely for fall out.

rdar://66603866
(cherry picked from commit 50a5c4f)

…ess (llvm#124712)

There are two logic bugs breaking RestrictMemoryToMaxAddress.
1. adding left_padding within MapDynamicShadow.
- RoundUpTo((uptr)free_begin + left_padding, alignment) already adjusts
for left padding. Adding this additionally within MapDynamicShadow
causes us to allocate a page larger than necessary.
- This incorrect calculation also means RestrictMemoryToMaxAddress will
never find a big enough gap.

2. There is also an issue with the expectation of hitting
KERN_INVALID_ADDRESS when we are beyond the addressable regions.
- For most embedded scenarios, we exceed vm_max_address without getting
KREN_INVALID_ADDRESS so we setting max_occupied_address to a memory
region the process doesn't have access to, beyond the max address, and
that space is never marked as available so we never find a valid gap in
those regions.
- At some point previous it seems the assumption was once we were beyond
the Max address we could expect KREN_INVALID_ADDRESS, which is no longer
true up through the extended space not given to most processes.
- Because of this, the check` if (new_max_vm < max_occupied_addr)` will
always fail and we will never restrict the address on smaller devices.
- Additionally because of the extra page added by adding left_padding,
and how we only minimally restrict the vm, there's a chance we restrict
the vm only enough for the correctly calculated size of shadow. In these
cases, restricting the vm max address and will always fail due to the
extra page added to space size.

credit to @delcypher for the left_padding diagnosis, remembered his old
radar and PR when investigating this. https://reviews.llvm.org/D85389

Will monitor closely for fall out.

rdar://66603866
(cherry picked from commit 50a5c4f)
@thetruestblue thetruestblue requested a review from a team as a code owner February 10, 2025 19:30
@thetruestblue thetruestblue requested a review from wrotki February 10, 2025 19:42
@thetruestblue
Copy link
Author

thetruestblue commented Feb 10, 2025

@wrotki I added you as reviewer but you also reviewed this previously.

@thetruestblue
Copy link
Author

@swift-ci test

Copy link

@wrotki wrotki left a comment

Choose a reason for hiding this comment

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

LGTM

@thetruestblue thetruestblue merged commit 6d9705c into stable/20240723 Feb 11, 2025
3 checks passed
@thetruestblue thetruestblue deleted the blue/cp-vm-restriction branch February 11, 2025 04:41
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