[PATCH 0/1] MdePkg/BaseLib: AArch64 SetJump/LongJump bugfix

[PATCH 0/1] MdePkg/BaseLib: AArch64 SetJump/LongJump bugfix

[jbobek]

Hi all,

I have recently discovered a bug in the BaseLib's implementation of
SetJump and LongJump; the offsets listed in the assembly files are 8
bytes off, causing the functions to read/write 8 bytes past the end of
the jump buffer. More details can be found in the commit message.

I must admit I am a bit surprised this has not bee caught before,
especially since the original implementation dates some 7 years back;
if there is something obvious that I am missing, please let me
know. Also, I am cc'ing all the people who signed off or reviewed the
original commit.

Note that this is my first contribution to EDK-II; I have tried to
follow the guidelines as closely as possible, but if there is still
something wrong with formatting etc., let me know and I shall submit a
v2 with the issues fixed. That being said, I did run PatchCheck.py and
it reported no errors.

Cheers!
 -Jan Bobek

Jan Bobek (1):
  MdePkg/BaseLib: Fix invalid memory access in AArch64 SetJump/LongJump

 MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S   | 8 ++++----
 MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

-- 
2.28.0

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

[Jan Bobek]

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>
---
 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.#
-- 
2.28.0

Re: [edk2-devel] [PATCH 0/1] MdePkg/BaseLib: AArch64 SetJump/LongJump bugfix

[Laszlo Ersek]

On 09/29/20 03:12, Jan Bobek wrote:
> Hi all,
> 
> I have recently discovered a bug in the BaseLib's implementation of
> SetJump and LongJump; the offsets listed in the assembly files are 8
> bytes off, causing the functions to read/write 8 bytes past the end of
> the jump buffer. More details can be found in the commit message.
> 
> I must admit I am a bit surprised this has not bee caught before,
> especially since the original implementation dates some 7 years back;
> if there is something obvious that I am missing, please let me
> know. Also, I am cc'ing all the people who signed off or reviewed the
> original commit.
> 
> Note that this is my first contribution to EDK-II; I have tried to
> follow the guidelines as closely as possible, but if there is still
> something wrong with formatting etc., let me know and I shall submit a
> v2 with the issues fixed. That being said, I did run PatchCheck.py and
> it reported no errors.
> 
> Cheers!
>  -Jan Bobek
> 
> Jan Bobek (1):
>   MdePkg/BaseLib: Fix invalid memory access in AArch64 SetJump/LongJump
> 
>  MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S   | 8 ++++----
>  MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 8 ++++----
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 

Please run the patches through "BaseTools/Scripts/GetMaintainer.py", for
determining the people who should be CC'd. For this series:

  Leif Lindholm <leif@nuviainc.com>
  Ard Biesheuvel <ard.biesheuvel@arm.com>
  Michael D Kinney <michael.d.kinney@intel.com>
  Liming Gao <gaoliming@byosoft.com.cn>
  Zhiguang Liu <zhiguang.liu@intel.com>

I'm correcting the CC list now.

Thanks,
Laszlo

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

[Laszlo Ersek]

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

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

[Ard Biesheuvel]

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
> 

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

[Jan Bobek]

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