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 B0650D80110 for ; Sat, 19 Aug 2023 02:50:08 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=JHHkhw9MToAW26IqDGMsrIzmrn9nDoKO97Rrhd3D/no=; 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=1692413407; v=1; b=IMaVQPQ33CvU6NnNSBO0UfCUlBS04jUO7XtPbD9Yw4aMA19vG6UQ5YPK1KvOs3bW9AiP8YsZ kdq2MZeCyVq1MlJQDBftniZhFehz6EDRs/n0llv+Fv9HxcGXloquntvYJhjDjr+GpjFHVCRDRWw ClTyPBE3uvvyvaibJ/gYXEBE= X-Received: by 127.0.0.2 with SMTP id QUNPYY7687511x6Q0OLXJK5U; Fri, 18 Aug 2023 19:50:07 -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.2790.1692413404587393747 for ; Fri, 18 Aug 2023 19:50:05 -0700 X-Received: from zrleap.intel-email.com (localhost [127.0.0.1]) by zrleap.intel-email.com (Postfix) with ESMTP id CFEA5A32E093 for ; Sat, 19 Aug 2023 10:49:59 +0800 (CST) X-Received: from localhost (localhost [127.0.0.1]) by zrleap.intel-email.com (Postfix) with ESMTP id B775EA32E06A for ; Sat, 19 Aug 2023 10:49:59 +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 003E7A32E08B for ; Sat, 19 Aug 2023 10:49:56 +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:49:53 +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> <177CA8C413C48EA2.21034@groups.io> In-Reply-To: <177CA8C413C48EA2.21034@groups.io> Subject: =?UTF-8?B?5Zue5aSNOiDlm57lpI06IGVkazItc3RhYmxlMjAyMzA4IFJlOiBbZWRrMi1kZXZlbF1bUEFUQ0ggdjEgMS8xXSBNZGVNb2R1bGVQa2c6IEhlYXBHdWFyZDogRG9uJ3QgQXNzdW1lIFBvb2wgSGVhZCBBbGxvY2F0ZWQgSW4gRmlyc3QgUGFnZQ==?= Date: Sat, 19 Aug 2023 10:49:53 +0800 Message-ID: <02a701d9d247$d4b84f60$7e28ee20$@byosoft.com.cn> MIME-Version: 1.0 Thread-Index: AQIIFSyDCjn1drWlb1YkGMWdRGm/8AITpndjAN6/ALACjEvyXwMZ0rYoAYAi47mvQ5/lEA== 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: L84NhoVgKUd0bOfqMVO8di8Gx7686176AA= 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=IMaVQPQ3; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=none Oliver: https://github.com/tianocore/edk2/pull/4751 is created to merge this patc= h.=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 gaoliming via > groups.io > =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2023=E5=B9=B48=E6=9C=8819=E6=97=A5 = 10:45 > =E6=94=B6=E4=BB=B6=E4=BA=BA: 'Oliver Smith-Denny' ; > 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: =E5=9B=9E=E5=A4=8D: =E5=9B=9E=E5=A4=8D: edk2-stable20= 2308 Re: [edk2-devel][PATCH v1 1/1] > MdeModulePkg: HeapGuard: Don't Assume Pool Head Allocated In First Page >=20 > Oliver: > This patch is not required to be updated. You can send another for this = clean > 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 ; deve= l@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= -devel][PATCH v1 1/1] > > MdeModulePkg: HeapGuard: Don't Assume Pool Head Allocated In First > Page > > > > On 8/15/2023 6:40 PM, gaoliming wrote: > > > 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 tag 202308. > > > > > > Besides, AdjustPoolHeadA() implementation has the extra logic " Si= ze > =3D > > ALIGN_VALUE (Size, 8);". Seemly, this logic is not required, because Si= ze has > > aligned by ALIGN_VARIABLE before enter into AdjustPoolHeadA. > > > > > > Thanks > > > Liming > > > > 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. > > > > Thanks, > > Oliver > > > > >> -----=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 ; Olive= r 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 = was > > >>>> allocated. This is not the case for ARM64 platforms when allocatin= g > > >>>> 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 po= ol is > > >>>> 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 n= ot > true > > >>>> 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 fr= ee 16 > > >>>> (or more depending) pages from the page the pool head was allocate= d > > 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 getti= ng 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 runt= ime > > >>>> memory). It should also cause all ARM64 platforms to fail in this > > >>>> configuration, for exactly the same reason, as this is core code m= aking > > >>>> 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 GuardAlignedToTai= l > > >>>> 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 und= o > 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 allocat= ion > > >> 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 >=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 (#107883): https://edk2.groups.io/g/devel/message/107883 Mute This Topic: https://groups.io/mt/100833735/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-