utils: Remove alignment in rva2offset() #491
Merged
Conversation
This file contains hidden or 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
rva2offset() currently aligns PointerToRawData down to a hardcoded 0x200 boundary. Unless malformed, the PointerToRawData is expected to be a multiple of the file alignment and the miniumum file alignment per the PE/COFF Specification is 512: https://learn.microsoft.com/windows/win32/debug/pe-format#optional-header-windows-specific-fields-image-only So, this alignment choice is understandable. UEFI images are based on the PE/COFF Specification and since they are stored in flash and flash space is limited, file alignments less than 512 are used and supported by linkers today. This proposes updating the logic to follow the formula: file_offset = PointerToRawData + (RVA - VirtualAddress) This allows both cases to be supported. While this does lose aligning an incorrectly reported pointer to raw data, that seems somewhat problematic anyway when tools, such as in this case, may report a value that is converted to an incorrect result. --- An example of where this was an issue was with the following values: - RVA (rva): 0xF6C8 - VirtualAddress (va): 0xD000 - PointerToRawData (ptrd): 0xBDA0 - File Alignment: 0x20 Previous Result: (rva - va) + aligned_pointer_to_raw_data(ptrd) (0xF6C8 - 0xD000) + aligned_pointer_to_raw_data(0xBDA0) 0x26C8 + 0xBC00 = 0xE2C8 (incorrect) Current Result: ptrd + (rva - va) 0xBDA0 + (0xF6C8 - 0xD000) 0xBDA0 + 0x26C8 = 0xE468 (correct) --- An example of where this was not previously an issue was with the following values: - RVA (rva): 0x13AB8 - VirtualAddress (va): 0x10000 - PointerToRawData (ptrd): 0xEC00 - File Alignment: 0x200 Previous Result: (rva - va) + aligned_pointer_to_raw_data(ptrd) (0x13AB8 - 0x10000) + aligned_pointer_to_raw_data(0xEC00) 0x3AB8 + 0xEC00 = 0x126B8 (correct) Current Result: ptrd + (rva - va) 0xEC00 + (0x13AB8 - 0x10000) 0xEC00 + 0x3AB8 = 0x126B8 (same outcome, also correct)
1 task
Owner
|
and thank you for the detailed writeup! ❤️ |
Owner
|
NB: non-breaking, in 0.10.2 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
rva2offset() currently aligns PointerToRawData down to a hardcoded 0x200 boundary. Unless malformed, the PointerToRawData is expected to be a multiple of the file alignment and the miniumum file alignment per the PE/COFF Specification is 512:
https://learn.microsoft.com/windows/win32/debug/pe-format#optional-header-windows-specific-fields-image-only
So, this alignment choice is understandable.
UEFI images are based on the PE/COFF Specification and since they are stored in flash and flash space is limited, file alignments less than 512 are used and supported by linkers today.
This proposes updating the logic to follow the formula:
This allows both cases to be supported. While this does lose aligning an incorrectly reported pointer to raw data, that seems problematic anyway when tools, such as in this case, may report a value that is converted to an incorrect result here.
An example of where this was an issue was with the following values:
Previous Result:
(rva - va) + aligned_pointer_to_raw_data(ptrd)
(0xF6C8 - 0xD000) + aligned_pointer_to_raw_data(0xBDA0)
0x26C8 + 0xBC00 = 0xE2C8 (incorrect)
Current Result:
ptrd + (rva - va)
0xBDA0 + (0xF6C8 - 0xD000)
0xBDA0 + 0x26C8 = 0xE468 (correct)
An example of where this was not previously an issue was with the following values:
Previous Result:
(rva - va) + aligned_pointer_to_raw_data(ptrd)
(0x13AB8 - 0x10000) + aligned_pointer_to_raw_data(0xEC00)
0x3AB8 + 0xEC00 = 0x126B8 (correct)
Current Result:
ptrd + (rva - va)
0xEC00 + (0x13AB8 - 0x10000)
0xEC00 + 0x3AB8 = 0x126B8 (same outcome, also correct)
This is made available for your consideration and feedback. Perhaps
options::ParseOptionscould have a field to enable this support. This change takes the simplest approach to reduce overall impact of the change and start the conversation.