Re: [edk2-devel] [PATCH 1/1] MdePkg/BaseLib: Fix invalid memory access in AArch64 SetJump/LongJump

["Jan Bobek" <jbobek@nvidia.com>]

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 <jbobek@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