On 10/1/20 3:04 PM, Laszlo Ersek wrote:
On 09/29/20 03:12, Jan Bobek wrote:Correct the memory offsets used in REG_ONE/REG_PAIR macros to synchronize them with definition of the BASE_LIBRARY_JUMP_BUFFER structure on AArch64.The REG_ONE macro declares only a single 64-bit register be read/written; however, the subsequent offset has previously been 16 bytes larger, creating an unused memory gap in the middle of the structure and causing SetJump/LongJump functions to read/write 8 bytes of memory past the end of the jump buffer struct. Signed-off-by: Jan Bobek <jbo...@nvidia.com>
Hello Jan, This is an excellent find - thanks for the patch.The reason this has gone unnoticed is because SetJump/LongJump are only used by the StartImage() and Exit() boot services (the latter uses LongJump to make the running image return from the former)
The jump buffer in question is allocated as follows: MdeModulePkg/Core/Dxe/Image/Image.c:1626:Image->JumpBuffer = AllocatePool (sizeof (BASE_LIBRARY_JUMP_BUFFER) + BASE_LIBRARY_JUMP_BUFFER_ALIGNMENT); Image->JumpContext = ALIGN_POINTER (Image->JumpBuffer, BASE_LIBRARY_JUMP_BUFFER_ALIGNMENT);
(apologies for whitespace soup - lines are often way too long in EDK2 code) where BASE_LIBRARY_JUMP_BUFFER_ALIGNMENT is defined as 8 for AArch64.AllocatePool() is guaranteed to return 8 byte aligned memory, and so the additional 8 bytes that are reserved for alignment are never needed, which is how we can write outside of the buffer unpunished.
So your patch is correct - please resend it according to the instructions provided by Laszlo. If you feel adventurous, you are welcome to propose some patches that remove BASE_LIBRARY_JUMP_BUFFER_ALIGNMENT entirely, as it serves no purpose other than make the code more difficult to understand (but please add a comment pointing out that the minimum alignment is guaranteed to be met, or perhaps we can do even better, and use a static assert that the natural alignment of the struct type is <= the guaranteed alignment of a pool allocation)
--- MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S | 8 ++++---- MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S index 72cea259e9..deefdf526b 100644 --- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S +++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S @@ -20,10 +20,10 @@ GCC_ASM_EXPORT(InternalLongJump) REG_ONE (x16, 96) /*IP0*/#define FPR_LAYOUT \ - REG_PAIR ( d8, d9, 112); \ - REG_PAIR (d10, d11, 128); \ - REG_PAIR (d12, d13, 144); \ - REG_PAIR (d14, d15, 160); + REG_PAIR ( d8, d9, 104); \ + REG_PAIR (d10, d11, 120); \ + REG_PAIR (d12, d13, 136); \ + REG_PAIR (d14, d15, 152);#/** # Saves the current CPU context that can be restored with a call to LongJump() and returns 0.# diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm index 20dd0f1b85..df70f29899 100644 --- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm +++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm @@ -19,10 +19,10 @@ REG_ONE (x16, #96) /*IP0*/#define FPR_LAYOUT \ - REG_PAIR ( d8, d9, #112); \ - REG_PAIR (d10, d11, #128); \ - REG_PAIR (d12, d13, #144); \ - REG_PAIR (d14, d15, #160); + REG_PAIR ( d8, d9, #104); \ + REG_PAIR (d10, d11, #120); \ + REG_PAIR (d12, d13, #136); \ + REG_PAIR (d14, d15, #152);;/** ; Saves the current CPU context that can be restored with a call to LongJump() and returns 0.#Your git setup is wrong as well (what with the extra line breaks in the formatted patch); please run "BaseTools/Scripts/SetupGit.py" in your edk2 clone. Updating the CC list on this one too. Thanks Laszlo
-=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#65804): https://edk2.groups.io/g/devel/message/65804 Mute This Topic: https://groups.io/mt/77195592/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
