From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from hqnvemgate25.nvidia.com (hqnvemgate25.nvidia.com [216.228.121.64]) by mx.groups.io with SMTP id smtpd.web12.1903.1601566762808756491 for ; Thu, 01 Oct 2020 08:39:22 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nvidia.com header.s=n1 header.b=aBeNcrqE; spf=permerror, err=parse error for token &{10 18 %{i}._ip.%{h}._ehlo.%{d}._spf.vali.email}: invalid domain name (domain: nvidia.com, ip: 216.228.121.64, mailfrom: jbobek@nvidia.com) Received: from hqmail.nvidia.com (Not Verified[216.228.121.13]) by hqnvemgate25.nvidia.com (using TLS: TLSv1.2, AES256-SHA) id ; Thu, 01 Oct 2020 08:38:30 -0700 Received: from localhost (10.124.1.5) by HQMAIL107.nvidia.com (172.20.187.13) with Microsoft SMTP Server (TLS) id 15.0.1473.3; Thu, 1 Oct 2020 15:39:15 +0000 References: <53c358a2-1e4f-45f5-4169-55809da1433d@arm.com> User-agent: mu4e 1.4.10; emacs 26.3 From: "Jan Bobek" To: Ard Biesheuvel , Laszlo Ersek , CC: Harry Liebel , Olivier Martin , Jeff Brasen , Ashish Singhal , Leif Lindholm , "Michael D Kinney" , Liming Gao , Zhiguang Liu Subject: Re: [edk2-devel] [PATCH 1/1] MdePkg/BaseLib: Fix invalid memory access in AArch64 SetJump/LongJump In-Reply-To: <53c358a2-1e4f-45f5-4169-55809da1433d@arm.com> Message-ID: <87zh56lzvi.fsf@nvidia.com> Date: Thu, 1 Oct 2020 09:39:13 -0600 MIME-Version: 1.0 Return-Path: jbobek@nvidia.com X-Originating-IP: [10.124.1.5] X-ClientProxiedBy: HQMAIL105.nvidia.com (172.20.187.12) To HQMAIL107.nvidia.com (172.20.187.13) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nvidia.com; s=n1; t=1601566710; bh=9SgVnzmJEkgmqmDFvNTSuGgQN1V+8/xoiR4O3vKgoMA=; h=References:User-agent:From:To:CC:Subject:In-Reply-To:Message-ID: Date:MIME-Version:Content-Type:X-Originating-IP:X-ClientProxiedBy; b=aBeNcrqE9XQ1h4nKUDjMjxjSeEr6ogG5YIRBBcPoHUHzCbsfQiGITBEh/1gak5Nii XFszotsr6jTZ61CR+UzIVLJWfCiJ4sC3VRgm7+XVYjJwaOY1O2v10iEmfIV7AQTnow /9iYhK7M0chnGnNnQzeB/osWsR1XgHXzofyQ5so5P8lVP+XRlLS/5jrsE1GMpm2JCE PPXS+Uq2aM2mk6QMoQp93T5ZRYrSaANC6LFa7lHJGXxH3BvzlL7gvfxersHij3cdnf 1RWQ9MjcjiNaBFLRLMjKC6qkREiAOZKUv1glD+c6+Jqscp3EcG5PuRFdmKSyfm97W6 JxgDc55Ql+zlA== Content-Type: text/plain 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 > > 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