From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id DD7E9AC1112 for ; Tue, 15 Aug 2023 10:58:58 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=V2/fh+O5fzTMaKILD73EA0d91qXij/IEdCmA3qEjtsM=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:Subject:To:CC:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1692097137; v=1; b=jxYlTF17/sJU1fe+m/jhFSyuIUowrzqU/lnMdccY9gskRADkRSAt1UnTr57SfdaL5UOyYK0A s+z0tWTtbtWn9Bk5a1zSpt/9uD846o7myA0DRXAFLugWu3wUxWiSMYx+KleURU6ng9De2xezObX lOKIyISxT2cRFFgjBHB6gxEE= X-Received: by 127.0.0.2 with SMTP id aIykYY7687511xHkFCaY4bOi; Tue, 15 Aug 2023 03:58:57 -0700 X-Received: from mx0a-0031df01.pphosted.com (mx0a-0031df01.pphosted.com [205.220.168.131]) by mx.groups.io with SMTP id smtpd.web10.131668.1692097136931345720 for ; Tue, 15 Aug 2023 03:58:56 -0700 X-Received: from pps.filterd (m0279864.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 37FAo3So007899; Tue, 15 Aug 2023 10:58:36 GMT X-Received: from nasanppmta01.qualcomm.com (i-global254.qualcomm.com [199.106.103.254]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3sfqp2sjrf-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 15 Aug 2023 10:58:36 +0000 X-Received: from nasanex01c.na.qualcomm.com (nasanex01c.na.qualcomm.com [10.45.79.139]) by NASANPPMTA01.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 37FAwZ6o015078 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 15 Aug 2023 10:58:35 GMT X-Received: from [10.251.41.163] (10.80.80.8) by nasanex01c.na.qualcomm.com (10.45.79.139) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.30; Tue, 15 Aug 2023 03:58:33 -0700 Message-ID: <7428ef3f-ed9b-6479-0b7f-be5a797fc7e0@quicinc.com> Date: Tue, 15 Aug 2023 11:58:10 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: edk2-stable202308 Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: HeapGuard: Don't Assume Pool Head Allocated In First Page To: Ard Biesheuvel , Oliver Smith-Denny , Liming Gao CC: , Jian J Wang , Dandan Bi , "Kinney, Michael D" , 'Andrew Fish' References: <20230809213457.18664-1-osde@linux.microsoft.com> From: "Leif Lindholm" In-Reply-To: X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01b.na.qualcomm.com (10.46.141.250) To nasanex01c.na.qualcomm.com (10.45.79.139) X-QCInternal: smtphost X-Proofpoint-GUID: 3d5oAOrpYWkWqm1rxOtLQQZqeVg2PJOf X-Proofpoint-ORIG-GUID: 3d5oAOrpYWkWqm1rxOtLQQZqeVg2PJOf Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,quic_llindhol@quicinc.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: Qv5Knqg4mbDqQjNIML3obfH5x7686176AA= Content-Language: en-GB Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=jxYlTF17; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=quicinc.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 2023-08-09 22:51, Ard Biesheuvel wrote: > On Wed, 9 Aug 2023 at 23:35, Oliver Smith-Denny > wrote: >> >> Currently, HeapGuard, when in the GuardAlignedToTail mode, assumes that >> the pool head has been allocated in the first page of memory that was >> allocated. This is not the case for ARM64 platforms when allocating >> runtime pools, as RUNTIME_PAGE_ALLOCATION_GRANULARITY is 64k, unlike >> X64, which has RUNTIME_PAGE_ALLOCATION_GRANULARITY as 4k. >> >> When a runtime pool is allocated on ARM64, the minimum number of pages >> allocated is 16, to match the runtime granularity. When a small pool is >> allocated and GuardAlignedToTail is true, HeapGuard instructs the pool >> head to be placed as (MemoryAllocated + EFI_PAGES_TO_SIZE(Number of Page= s) >> - SizeRequiredForPool). >> >> This gives this scenario: >> >> |Head Guard|Large Free Number of Pages|PoolHead|TailGuard| >> >> When this pool goes to be freed, HeapGuard instructs the pool code to >> free from (PoolHead & ~EFI_PAGE_MASK). However, this assumes that the >> PoolHead is in the first page allocated, which as shown above is not tru= e >> in this case. For the 4k granularity case (i.e. where the correct number= of >> pages are allocated for this pool), this logic does work. >> >> In this failing case, HeapGuard then instructs the pool code to free 16 >> (or more depending) pages from the page the pool head was allocated on, >> which as seen above means we overrun the pool and attempt to free memory >> far past the pool. We end up running into the tail guard and getting an >> access flag fault. >> >> This causes ArmVirtQemu to fail to boot with an access flag fault when >> GuardAlignedToTail is set to true (and pool guard enabled for runtime >> memory). It should also cause all ARM64 platforms to fail in this >> configuration, for exactly the same reason, as this is core code making >> the assumption. >> >> This patch removes HeapGuard's assumption that the pool head is allocate= d >> on the first page and instead undoes the same logic that HeapGuard did >> when allocating the pool head in the first place. >> >> With this patch in place, ArmVirtQemu boots with GuardAlignedToTail >> set to true (and when it is false, also). >> >> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4521 >> Github PR: https://github.com/tianocore/edk2/pull/4731 >> >> Cc: Leif Lindholm >> Cc: Ard Biesheuvel >> Cc: Jian J Wang >> Cc: Liming Gao >> Cc: Dandan Bi >> >> Signed-off-by: Oliver Smith-Denny >=20 > Thanks a lot for fixing this - I ran into this a while ago but didn't > quite figure out what exactly was going wrong, although the runtime > allocation granularity was obviously a factor here. >=20 > Reviewed-by: Ard Biesheuvel >=20 > Can we include this in the stable tag please? Bugfix, submitted during soft freeze. I think it can go in. *but* this affects !AARCH64 also - need a reaction from MdeModulePkg=20 maintainers. Acked-by: Leif Lindholm >> --- >> MdeModulePkg/Core/Dxe/Mem/HeapGuard.h | 7 ++++++- >> MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 14 +++++++++++--- >> MdeModulePkg/Core/Dxe/Mem/Pool.c | 2 +- >> 3 files changed, 18 insertions(+), 5 deletions(-) >> >> diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h b/MdeModulePkg/Core/D= xe/Mem/HeapGuard.h >> index 9a32b4dd51d5..24b4206c0e02 100644 >> --- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h >> +++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h >> @@ -378,12 +378,17 @@ AdjustPoolHeadA ( >> Get the page base address according to pool head address. >> >> @param[in] Memory Head address of pool to free. >> + @param[in] NoPages Number of pages actually allocated. >> + @param[in] Size Size of memory requested. >> + (plus pool head/tail overhead) >> >> @return Address of pool head. >> **/ >> VOID * >> AdjustPoolHeadF ( >> - IN EFI_PHYSICAL_ADDRESS Memory >> + IN EFI_PHYSICAL_ADDRESS Memory, >> + IN UINTN NoPages, >> + IN UINTN Size >> ); >> >> /** >> diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c b/MdeModulePkg/Core/D= xe/Mem/HeapGuard.c >> index 9377f620c5a5..0c0ca61872b4 100644 >> --- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c >> +++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c >> @@ -1037,12 +1037,17 @@ AdjustPoolHeadA ( >> Get the page base address according to pool head address. >> >> @param[in] Memory Head address of pool to free. >> + @param[in] NoPages Number of pages actually allocated. >> + @param[in] Size Size of memory requested. >> + (plus pool head/tail overhead) >> >> @return Address of pool head. >> **/ >> VOID * >> AdjustPoolHeadF ( >> - IN EFI_PHYSICAL_ADDRESS Memory >> + IN EFI_PHYSICAL_ADDRESS Memory, >> + IN UINTN NoPages, >> + IN UINTN Size >> ) >> { >> if ((Memory =3D=3D 0) || ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7= ) !=3D 0)) { >> @@ -1053,9 +1058,12 @@ AdjustPoolHeadF ( >> } >> >> // >> - // Pool head is put near the tail Guard >> + // Pool head is put near the tail Guard. We need to exactly undo the = addition done in AdjustPoolHeadA >> + // because we may not have allocated the pool head on the first alloc= ated page, since we are aligned to >> + // the tail and on some architectures, the runtime page allocation gr= anularity is > one page. So we allocate >> + // more pages than we need and put the pool head somewhere past the f= irst page. >> // >> - return (VOID *)(UINTN)(Memory & ~EFI_PAGE_MASK); >> + return (VOID *)(UINTN)(Memory + Size - EFI_PAGES_TO_SIZE (NoPages)); >> } >> >> /** >> diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Me= m/Pool.c >> index b20cbfdedbab..716dd045f9fd 100644 >> --- a/MdeModulePkg/Core/Dxe/Mem/Pool.c >> +++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c >> @@ -783,7 +783,7 @@ CoreFreePoolI ( >> NoPages =3D EFI_SIZE_TO_PAGES (Size) + EFI_SIZE_TO_PAGES (Granula= rity) - 1; >> NoPages &=3D ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1); >> if (IsGuarded) { >> - Head =3D AdjustPoolHeadF ((EFI_PHYSICAL_ADDRESS)(UINTN)Head); >> + Head =3D AdjustPoolHeadF ((EFI_PHYSICAL_ADDRESS)(UINTN)Head, NoPa= ges, Size); >> CoreFreePoolPagesWithGuard ( >> Pool->MemoryType, >> (EFI_PHYSICAL_ADDRESS)(UINTN)Head, >> -- >> 2.40.1 >> -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#107765): https://edk2.groups.io/g/devel/message/107765 Mute This Topic: https://groups.io/mt/100755729/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-