Ard Biesheuvel writes: > 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.
The way I ran into this was due to use of SetJump/LongJump in UnitTestFrameworkPkg; specifically, a jump buffer is statically allocated right next to the unit test framework handle: UnitTestFrameworkPkg/Library/UnitTestLib/RunTests.c:15: STATIC UNIT_TEST_FRAMEWORK_HANDLE mFrameworkHandle = NULL; BASE_LIBRARY_JUMP_BUFFER gUnitTestJumpBuffer; My compiler actually places the framework handle *after* the jump buffer; as soon as SetJump was called on gUnitTestJumpBuffer (prior to running a test), mFrameworkHandle got overwritten with zeros, causing an ASSERT fail in GetActiveFrameworkHandle(). > 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) I am in fact quite new to the EDK-II codebase, so I would rather fix one thing at a time if that's okay. I shall fix my git setup using the script Laszlo pointed out and resend the patch. Thank you so much for your feedback, gentlemen! -Jan -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#65806): https://edk2.groups.io/g/devel/message/65806 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] -=-=-=-=-=-=-=-=-=-=-=-
