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 BC63E780091 for ; Sat, 19 Aug 2023 02:45:21 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=DiPciMgqb4C42Dn8jHhBQYORPtUJ77dNelwKhLU/GVU=; 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=1692413120; v=1; b=u8u63Qtsz/+8MUDcsXKuB896POz12VtJDTRcG9M6uh2i0q7du7bs28pIBfBFOiDHeuPdJj8U HjnSDLfkeR+/DkoW2dv5rBwRoLd2U3Itx3A64F6H6u0zcyl1FIA5MNTgmUyyPwIq7dd/EWteOKn r+YMaGE4bf4q2BsQ+0Yd7yC0= X-Received: by 127.0.0.2 with SMTP id gxkVYY7687511xx95s2Ms4aZ; Fri, 18 Aug 2023 19:45:20 -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.2739.1692413117667785988 for ; Fri, 18 Aug 2023 19:45:19 -0700 X-Received: from zrleap.intel-email.com (localhost [127.0.0.1]) by zrleap.intel-email.com (Postfix) with ESMTP id 6C291A32E08B for ; Sat, 19 Aug 2023 10:45:14 +0800 (CST) X-Received: from localhost (localhost [127.0.0.1]) by zrleap.intel-email.com (Postfix) with ESMTP id 4DAC0A32E05B for ; Sat, 19 Aug 2023 10:45:14 +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 8CDD3A32E074 for ; Sat, 19 Aug 2023 10:45:11 +0800 (CST) X-Received: from DESKTOPS6D0PVI ([172.16.100.21]) (envelope-sender ) by 192.168.6.13 with ESMTP for ; Sat, 19 Aug 2023 10:45:07 +0800 X-WM-Sender: gaoliming@byosoft.com.cn X-Originating-IP: 172.16.100.21 X-WM-AuthFlag: YES X-WM-AuthUser: gaoliming@byosoft.com.cn From: "gaoliming via groups.io" To: "'Oliver Smith-Denny'" , , , "'Ard Biesheuvel'" 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> <010b01d9cfe2$a22bd380$e6837a80$@byosoft.com.cn> In-Reply-To: Subject: =?UTF-8?B?5Zue5aSNOiDlm57lpI06IGVkazItc3RhYmxlMjAyMzA4IFJlOiBbZWRrMi1kZXZlbF1bUEFUQ0ggdjEgMS8xXSBNZGVNb2R1bGVQa2c6IEhlYXBHdWFyZDogRG9uJ3QgQXNzdW1lIFBvb2wgSGVhZCBBbGxvY2F0ZWQgSW4gRmlyc3QgUGFnZQ==?= Date: Sat, 19 Aug 2023 10:45:06 +0800 Message-ID: <02a601d9d247$2a139e20$7e3ada60$@byosoft.com.cn> MIME-Version: 1.0 Thread-Index: AQIIFSyDCjn1drWlb1YkGMWdRGm/8AITpndjAN6/ALACjEvyXwMZ0rYor0+ecuA= 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: 1PGa8xuaapcDMgzqxVUnvAgQx7686176AA= 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=u8u63Qts; 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 patch is not required to be updated. You can send another for this cl= ean up in Allocate after the stable tag.=20 Thanks Liming > -----=E9=82=AE=E4=BB=B6=E5=8E=9F=E4=BB=B6----- > =E5=8F=91=E4=BB=B6=E4=BA=BA: Oliver Smith-Denny > =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2023=E5=B9=B48=E6=9C=8818=E6=97=A5 = 1:05 > =E6=94=B6=E4=BB=B6=E4=BA=BA: gaoliming ; devel@= edk2.groups.io; > quic_llindhol@quicinc.com; 'Ard Biesheuvel' > =E6=8A=84=E9=80=81: 'Jian J Wang' ; 'Dandan Bi' > ; 'Kinney, Michael D' ; > 'Andrew Fish' > =E4=B8=BB=E9=A2=98: Re: =E5=9B=9E=E5=A4=8D: edk2-stable202308 Re: [edk2-d= evel][PATCH v1 1/1] > MdeModulePkg: HeapGuard: Don't Assume Pool Head Allocated In First Page >=20 > On 8/15/2023 6:40 PM, gaoliming wrote: > > Oliver: > > This change reverts the changes done in AdjustPoolHeadA(). It matche= s > the allocation logic. I think this change is good. Reviewed-by: Liming Ga= o > I am also OK to merge this change for the > stable tag 202308. > > > > Besides, AdjustPoolHeadA() implementation has the extra logic " Size= =3D > ALIGN_VALUE (Size, 8);". Seemly, this logic is not required, because Size= has > aligned by ALIGN_VARIABLE before enter into AdjustPoolHeadA. > > > > Thanks > > Liming >=20 > Thanks for the review, Liming. Looking at the alignment code, I agree, > the ALIGN_VALUE doesn't seem to be needed. Do you want me to send a v2 > with that dropped or take the patch as is? Looks like we have the > required reviewers and probably no further folks reviewing. >=20 > Thanks, > Oliver >=20 > >> -----=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 = Smith-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] > MdeModulePkg: > >> HeapGuard: Don't Assume Pool Head Allocated In First Page > >> > >> 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 wa= s > >>>> 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 po= ol > >>>> 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 t= o > >>>> free from (PoolHead & ~EFI_PAGE_MASK). However, this assumes that > the > >>>> PoolHead is in the first page allocated, which as shown above is not= true > >>>> in this case. For the 4k granularity case (i.e. where the correct nu= mber 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 wh= en > >>>> GuardAlignedToTail is set to true (and pool guard enabled for runtim= e > >>>> memory). It should also cause all ARM64 platforms to fail in this > >>>> configuration, for exactly the same reason, as this is core code mak= ing > >>>> 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 d= id > >>>> 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? > >> > >> Bugfix, submitted during soft freeze. I think it can go in. > >> *but* this affects !AARCH64 also - need a reaction from MdeModulePkg > >> 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/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 = the > >> 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 allocatio= n > >> 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 > >> > > > > -=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 (#107882): https://edk2.groups.io/g/devel/message/107882 Mute This Topic: https://groups.io/mt/100833709/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-