From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.12622.1601558242906341355 for ; Thu, 01 Oct 2020 06:17:23 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 83D3D30E; Thu, 1 Oct 2020 06:17:21 -0700 (PDT) Received: from [192.168.1.81] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 638473F6CF; Thu, 1 Oct 2020 06:17:19 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH 1/1] MdePkg/BaseLib: Fix invalid memory access in AArch64 SetJump/LongJump To: Laszlo Ersek , devel@edk2.groups.io, jbobek@nvidia.com Cc: Harry Liebel , Olivier Martin , Jeff Brasen , Ashish Singhal , Leif Lindholm , Michael D Kinney , Liming Gao , Zhiguang Liu References: From: "Ard Biesheuvel" Message-ID: <53c358a2-1e4f-45f5-4169-55809da1433d@arm.com> Date: Thu, 1 Oct 2020 15:17:17 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 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 >