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

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

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.

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
>