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 CFB54940D0A for ; Wed, 16 Aug 2023 01:40:39 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=pJf4ADNMgbq9KYU7+k7p9j5L0Z0hzPsSTgw6y7H04hk=; c=relaxed/simple; d=groups.io; h=From:To:Cc:References:In-Reply-To:Subject:Date:Message-ID:MIME-Version:Thread-Index:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Transfer-Encoding:Content-Language; s=20140610; t=1692150038; v=1; b=JFTt57Qcv3TQ0Pt3Ua5Mz04pLIhhyUBO3+bEaI8aLSWDmxFjAd51q8Nn+g1mivpNXWSPG4a2 mxeprW8/tcp6TxWNSS9Dhxri7R158hUqDdws+pQ+QC1rZFmSujGGpciMMZMXp2d/AhfyPNJ2hIr ityX1SjkKsa+XQFpAKk16254= X-Received: by 127.0.0.2 with SMTP id 3FGxYY7687511xc6KOsavFFX; Tue, 15 Aug 2023 18:40:38 -0700 X-Received: from zrleap.intel-email.com (zrleap.intel-email.com [114.80.218.36]) by mx.groups.io with SMTP id smtpd.web11.151755.1692150036223815514 for ; Tue, 15 Aug 2023 18:40:37 -0700 X-Received: from zrleap.intel-email.com (localhost [127.0.0.1]) by zrleap.intel-email.com (Postfix) with ESMTP id BFB48A32E027 for ; Wed, 16 Aug 2023 09:40:33 +0800 (CST) X-Received: from localhost (localhost [127.0.0.1]) by zrleap.intel-email.com (Postfix) with ESMTP id AEDF9A32E01B for ; Wed, 16 Aug 2023 09:40:33 +0800 (CST) X-Received: from mail.byosoft.com.cn (mail.byosoft.com.cn [58.240.74.242]) by zrleap.intel-email.com (Postfix) with SMTP id EFD5AA32E023 for ; Wed, 16 Aug 2023 09:40:30 +0800 (CST) X-Received: from DESKTOPS6D0PVI ([58.246.60.130]) (envelope-sender ) by 192.168.6.13 with ESMTP for ; Wed, 16 Aug 2023 09:40:24 +0800 X-WM-Sender: gaoliming@byosoft.com.cn X-Originating-IP: 58.246.60.130 X-WM-AuthFlag: YES X-WM-AuthUser: gaoliming@byosoft.com.cn From: "gaoliming via groups.io" To: , , "'Ard Biesheuvel'" , "'Oliver Smith-Denny'" Cc: "'Jian J Wang'" , "'Dandan Bi'" , "'Kinney, Michael D'" , "'Andrew Fish'" References: <20230809213457.18664-1-osde@linux.microsoft.com> <7428ef3f-ed9b-6479-0b7f-be5a797fc7e0@quicinc.com> In-Reply-To: <7428ef3f-ed9b-6479-0b7f-be5a797fc7e0@quicinc.com> Subject: =?UTF-8?B?5Zue5aSNOiBlZGsyLXN0YWJsZTIwMjMwOCBSZTogW2VkazItZGV2ZWxdW1BBVENIIHYxIDEvMV0gTWRlTW9kdWxlUGtnOiBIZWFwR3VhcmQ6IERvbid0IEFzc3VtZSBQb29sIEhlYWQgQWxsb2NhdGVkIEluIEZpcnN0IFBhZ2U=?= Date: Wed, 16 Aug 2023 09:40:27 +0800 Message-ID: <010b01d9cfe2$a22bd380$e6837a80$@byosoft.com.cn> MIME-Version: 1.0 Thread-Index: AQIIFSyDCjn1drWlb1YkGMWdRGm/8AITpndjAN6/ALCveAXgsA== 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,gaoliming@byosoft.com.cn List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: 85sHSjpXe4qo5b13riXVL3AMx7686176AA= Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Content-Language: zh-cn X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=JFTt57Qc; dmarc=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 Oliver: This change reverts the changes done in AdjustPoolHeadA(). It matches the= allocation logic. I think this change is good. Reviewed-by: Liming Gao I am also OK to merge this change for the stable ta= g 202308.=20 Besides, AdjustPoolHeadA() implementation has the extra logic " Size =3D = ALIGN_VALUE (Size, 8);". Seemly, this logic is not required, because Size h= as aligned by ALIGN_VARIABLE before enter into AdjustPoolHeadA. =20 Thanks Liming > -----=E9=82=AE=E4=BB=B6=E5=8E=9F=E4=BB=B6----- > =E5=8F=91=E4=BB=B6=E4=BA=BA: devel@edk2.groups.io = =E4=BB=A3=E8=A1=A8 Leif Lindholm > =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2023=E5=B9=B48=E6=9C=8815=E6=97=A5 = 18:58 > =E6=94=B6=E4=BB=B6=E4=BA=BA: Ard Biesheuvel ; Oliver Smi= th-Denny > ; Liming Gao > =E6=8A=84=E9=80=81: devel@edk2.groups.io; Jian J Wang ; Dandan > Bi ; Kinney, Michael D ; > 'Andrew Fish' > =E4=B8=BB=E9=A2=98: edk2-stable202308 Re: [edk2-devel][PATCH v1 1/1] MdeM= odulePkg: > HeapGuard: Don't Assume Pool Head Allocated In First Page >=20 > 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 tha= t > >> 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 i= s > >> allocated and GuardAlignedToTail is true, HeapGuard instructs the pool > >> head to be placed as (MemoryAllocated + EFI_PAGES_TO_SIZE(Number of > Pages) > >> - 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 t= rue > >> in this case. For the 4k granularity case (i.e. where the correct numb= er of > >> pages are allocated for this pool), this logic does work. > >> > >> In this failing case, HeapGuard then instructs the pool code to free 1= 6 > >> (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 a= n > >> 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 makin= g > >> the assumption. > >> > >> This patch removes HeapGuard's assumption that the pool head is > allocated > >> 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 > > > > 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. > > > > Reviewed-by: Ard Biesheuvel > > > > Can we include this in the stable tag please? >=20 > Bugfix, submitted during soft freeze. I think it can go in. > *but* this affects !AARCH64 also - need a reaction from MdeModulePkg > maintainers. >=20 > Acked-by: Leif Lindholm >=20 > >> --- > >> 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/Dxe/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/Dxe/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 th= e > addition done in AdjustPoolHeadA > >> + // because we may not have allocated the pool head on the first > allocated page, since we are aligned to > >> + // the tail and on some architectures, the runtime page allocation > granularity is > one page. So we allocate > >> + // more pages than we need and put the pool head somewhere past > the first 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/Mem/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 > (Granularity) - 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, > NoPages, Size); > >> CoreFreePoolPagesWithGuard ( > >> Pool->MemoryType, > >> (EFI_PHYSICAL_ADDRESS)(UINTN)Head, > >> -- > >> 2.40.1 > >> >=20 >=20 >=20 >=20 >=20 -=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 (#107777): https://edk2.groups.io/g/devel/message/107777 Mute This Topic: https://groups.io/mt/100771741/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-