Align all memory used by the system symbol table #297
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Rarely, the compiler/linker will decide to put `gSystemSymbolMemory` on a smaller alignment boundary (e.g. 4 bytes) than what is required for the data it contains. `gSystemSymbolMemory` is declared as a `char[]`, but is really being used as generic backing memory for the system symbol table and any other data structures associated with it. Notably, the very first data structure placed in the `gSystemSymbolMemory` is an `ION_ALLOCATION_CHAIN`, which may need a wider alignment. With the current behavior, an `ION_ALLOCATION_CHAIN` could be allocated at a smaller alignment than needed. The `ION_ALLOC_BLOCK_TO_USER_PTR` macro then gets an aligned offset into the memory chunk associated with the `ION_ALLOCATION_CHAIN`. When another allocation needs to be performed, the `ION_ALLOC_USER_PTR_TO_BLOCK` macro is used to convert the user data pointer into an `ION_ALLOCATION_CHAIN`, but this is where the problems start happening. Because `ION_ALLOC_BLOCK_TO_USER_PTR` aligns the user data pointer, an unknown amount of padding exists between the end of the `ION_ALLOCATION_CHAIN` and the user data pointer (up to `ALLOC_ALIGNMENT` bytes). `ION_ALLOC_USER_PTR_TO_BLOCK` doesn't account for any alignment done to the user data pointer (and there's no way it can really), meaning it might produce a pointer that is offset from the actual `ION_ALLOCATION_CHAIN` by the amount of padding that exists. This usually leads to a crash, since the data being read is now scrambled. This commit addresses this issue by introducing an `ALIGN_AS` macro that forces a data structure to be aligned to a wider alignment than the type requires. This is applied to the `gSystemSymbolMemory` array, so that it is always located at a correctly aligned address. This also adds a few debug assertions to the system symbol table allocation routines and gets rid of the pointer aligning behavior in the `ION_ALLOC` converter macros (an offset from a pointer should be aligned if you start with an aligned pointer and add an aligned offset to it; in the case that you start with a misaligned pointer, you'll at least get the right pointer back with a round trip conversion, rather than a misaligned one).
tgregg
approved these changes
Jun 22, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available: N/A
Description of changes:
Rarely, the compiler/linker will decide to put
gSystemSymbolMemoryon a smaller alignment boundary (e.g. 4 bytes) than what is required for the data it contains.gSystemSymbolMemoryis declared as achar[], but is really being used as generic backing memory for the system symbol table and any other data structures associated with it. Notably, the very first data structure placed in thegSystemSymbolMemoryis anION_ALLOCATION_CHAIN, which may need a wider alignment.With the current behavior, an
ION_ALLOCATION_CHAINcould be allocated at a smaller alignment than needed. TheION_ALLOC_BLOCK_TO_USER_PTRmacro then gets an aligned offset into the memory chunk associated with theION_ALLOCATION_CHAIN. When another allocation needs to be performed, theION_ALLOC_USER_PTR_TO_BLOCKmacro is used to convert the user data pointer into anION_ALLOCATION_CHAIN, but this is where the problems start happening. BecauseION_ALLOC_BLOCK_TO_USER_PTRaligns the user data pointer, an unknown amount of padding exists between the end of theION_ALLOCATION_CHAINand the user data pointer (up toALLOC_ALIGNMENTbytes).ION_ALLOC_USER_PTR_TO_BLOCKdoesn't account for any alignment done to the user data pointer (and there's no way it can really), meaning it might produce a pointer that is offset from the actualION_ALLOCATION_CHAINby the amount of padding that exists. This usually leads to a crash, since the data being read is now scrambled.This commit addresses this issue by introducing an
ALIGN_ASmacro that forces a data structure to be aligned to a wider alignment than the type requires. This is applied to thegSystemSymbolMemoryarray, so that it is always located at a correctly aligned address. This also adds a few debug assertions to the system symbol table allocation routines and gets rid of the pointer aligning behavior in theION_ALLOCconverter macros (an offset from a pointer should be aligned if you start with an aligned pointer and add an aligned offset to it; in the case that you start with a misaligned pointer, you'll at least get the right pointer back with a round trip conversion, rather than a misaligned one).By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.