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 4A0A3AC1779 for ; Thu, 17 Aug 2023 17:05:26 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=AWFlYJj8LVVp9AQfT4aEhv02RRP6nfImm1eSN8Ka9ck=; c=relaxed/simple; d=groups.io; h=DKIM-Filter: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=1692291924; v=1; b=j/+SQP2hYXbwudwU0ae7xArWf/+A9X5PLU78k/2yOm3bxa571taxVOPvES/1FBS/qC+gLBBx kyFAS6+Gft7rBh11hgh8di/m3YslIRzh5puAzU8zWdKp8L1rWFvcgPLwiiR4yb4vy6bRJgbefvJ GFRPy+o7cKXQoX8iVGVjhXqk= X-Received: by 127.0.0.2 with SMTP id m7TWYY7687511x9BsHPE2bAJ; Thu, 17 Aug 2023 10:05:24 -0700 X-Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web11.214.1692291924143544444 for ; Thu, 17 Aug 2023 10:05:24 -0700 X-Received: from [10.137.194.171] (unknown [131.107.1.171]) by linux.microsoft.com (Postfix) with ESMTPSA id A292A211F7B5; Thu, 17 Aug 2023 10:05:23 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com A292A211F7B5 Message-ID: Date: Thu, 17 Aug 2023 10:05:23 -0700 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: =?UTF-8?B?UmU6IOWbnuWkjTogZWRrMi1zdGFibGUyMDIzMDggUmU6IFtlZGsyLWRldmVsXVtQQVRDSCB2MSAxLzFdIE1kZU1vZHVsZVBrZzogSGVhcEd1YXJkOiBEb24ndCBBc3N1bWUgUG9vbCBIZWFkIEFsbG9jYXRlZCBJbiBGaXJzdCBQYWdl?= To: gaoliming , devel@edk2.groups.io, quic_llindhol@quicinc.com, '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> From: "Oliver Smith-Denny" In-Reply-To: <010b01d9cfe2$a22bd380$e6837a80$@byosoft.com.cn> 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,osde@linux.microsoft.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: 3kmB7rW6F1qO3iPCm5RCXrAnx7686176AA= Content-Language: en-US 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="j/+SQP2h"; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=linux.microsoft.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 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. >=20 > Besides, AdjustPoolHeadA() implementation has the extra logic " Size = =3D ALIGN_VALUE (Size, 8);". Seemly, this logic is not required, because Si= ze has aligned by ALIGN_VARIABLE before enter into AdjustPoolHeadA. > =20 > 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 ; Oliver Sm= ith-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] Mde= ModulePkg: >> 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 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? >> >> 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 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 -=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 (#107836): https://edk2.groups.io/g/devel/message/107836 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-