* [PATCH 0/1] MdePkg/BaseLib: AArch64 SetJump/LongJump bugfix @ 2020-09-29 1:12 jbobek 2020-09-29 1:12 ` [PATCH 1/1] MdePkg/BaseLib: Fix invalid memory access in AArch64 SetJump/LongJump Jan Bobek 2020-10-01 13:02 ` [edk2-devel] [PATCH 0/1] MdePkg/BaseLib: AArch64 SetJump/LongJump bugfix Laszlo Ersek 0 siblings, 2 replies; 6+ messages in thread From: jbobek @ 2020-09-29 1:12 UTC (permalink / raw) To: devel; +Cc: Harry Liebel, Olivier Martin, Liming Gao, Jeff Brasen, Ashish Singhal 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] MdePkg/BaseLib: Fix invalid memory access in AArch64 SetJump/LongJump 2020-09-29 1:12 [PATCH 0/1] MdePkg/BaseLib: AArch64 SetJump/LongJump bugfix jbobek @ 2020-09-29 1:12 ` Jan Bobek 2020-10-01 13:04 ` [edk2-devel] " Laszlo Ersek 2020-10-01 13:02 ` [edk2-devel] [PATCH 0/1] MdePkg/BaseLib: AArch64 SetJump/LongJump bugfix Laszlo Ersek 1 sibling, 1 reply; 6+ messages in thread From: Jan Bobek @ 2020-09-29 1:12 UTC (permalink / raw) To: devel; +Cc: Harry Liebel, Olivier Martin, Liming Gao, Jeff Brasen, Ashish Singhal 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] MdePkg/BaseLib: Fix invalid memory access in AArch64 SetJump/LongJump 2020-09-29 1:12 ` [PATCH 1/1] MdePkg/BaseLib: Fix invalid memory access in AArch64 SetJump/LongJump Jan Bobek @ 2020-10-01 13:04 ` Laszlo Ersek 2020-10-01 13:17 ` Ard Biesheuvel 0 siblings, 1 reply; 6+ messages in thread From: Laszlo Ersek @ 2020-10-01 13:04 UTC (permalink / raw) To: devel, jbobek Cc: Harry Liebel, Olivier Martin, Jeff Brasen, Ashish Singhal, Leif Lindholm, Ard Biesheuvel, Michael D Kinney, Liming Gao, Zhiguang Liu 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] MdePkg/BaseLib: Fix invalid memory access in AArch64 SetJump/LongJump 2020-10-01 13:04 ` [edk2-devel] " Laszlo Ersek @ 2020-10-01 13:17 ` Ard Biesheuvel 2020-10-01 15:39 ` Jan Bobek 0 siblings, 1 reply; 6+ messages in thread From: Ard Biesheuvel @ 2020-10-01 13:17 UTC (permalink / raw) To: Laszlo Ersek, devel, jbobek Cc: Harry Liebel, Olivier Martin, Jeff Brasen, Ashish Singhal, Leif Lindholm, Michael D Kinney, Liming Gao, Zhiguang Liu 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] MdePkg/BaseLib: Fix invalid memory access in AArch64 SetJump/LongJump 2020-10-01 13:17 ` Ard Biesheuvel @ 2020-10-01 15:39 ` Jan Bobek 0 siblings, 0 replies; 6+ messages in thread From: Jan Bobek @ 2020-10-01 15:39 UTC (permalink / raw) To: Ard Biesheuvel, Laszlo Ersek, devel Cc: Harry Liebel, Olivier Martin, Jeff Brasen, Ashish Singhal, Leif Lindholm, Michael D Kinney, Liming Gao, Zhiguang Liu 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH 0/1] MdePkg/BaseLib: AArch64 SetJump/LongJump bugfix 2020-09-29 1:12 [PATCH 0/1] MdePkg/BaseLib: AArch64 SetJump/LongJump bugfix jbobek 2020-09-29 1:12 ` [PATCH 1/1] MdePkg/BaseLib: Fix invalid memory access in AArch64 SetJump/LongJump Jan Bobek @ 2020-10-01 13:02 ` Laszlo Ersek 1 sibling, 0 replies; 6+ messages in thread From: Laszlo Ersek @ 2020-10-01 13:02 UTC (permalink / raw) To: devel, jbobek Cc: Harry Liebel, Olivier Martin, Liming Gao, Jeff Brasen, Ashish Singhal, Leif Lindholm, Ard Biesheuvel, Michael D Kinney, Zhiguang Liu 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-01 15:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-09-29 1:12 [PATCH 0/1] MdePkg/BaseLib: AArch64 SetJump/LongJump bugfix jbobek 2020-09-29 1:12 ` [PATCH 1/1] MdePkg/BaseLib: Fix invalid memory access in AArch64 SetJump/LongJump Jan Bobek 2020-10-01 13:04 ` [edk2-devel] " Laszlo Ersek 2020-10-01 13:17 ` Ard Biesheuvel 2020-10-01 15:39 ` Jan Bobek 2020-10-01 13:02 ` [edk2-devel] [PATCH 0/1] MdePkg/BaseLib: AArch64 SetJump/LongJump bugfix Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox