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 3E21DAC198B for ; Thu, 15 Feb 2024 17:08:45 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=PT5LNAQ/IKNaIB8Vf+sryLHv8c0kaIQdzp26JGgDxwI=; 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=1708016923; v=1; b=Nf3RgP+jobq5PK/MAE48N1zA2bg6tpK1clHlvSFC08xsCFGYQMNfcmrLeWZcdbWEURdEQ/OP FmZPI1MfMCenTrQFdC6huXsf66CEi9i4JQDwgUILdL0VVRLYzTZy/sUU1G1DucXxJ5rETqySJqx o12aogcaJsj7ekKpDNhkF9Do= X-Received: by 127.0.0.2 with SMTP id 545IYY7687511x9EBAgIMO1y; Thu, 15 Feb 2024 09:08:43 -0800 X-Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web11.19629.1708016923334520745 for ; Thu, 15 Feb 2024 09:08:43 -0800 X-Received: from [10.137.194.171] (unknown [131.107.160.171]) by linux.microsoft.com (Postfix) with ESMTPSA id CF3FF20B2000; Thu, 15 Feb 2024 09:08:42 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com CF3FF20B2000 Message-ID: <7f65d4af-898e-437f-b31c-52156c6a696c@linux.microsoft.com> Date: Thu, 15 Feb 2024 09:08:42 -0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: DxeCore: Don't Guard Large Runtime Granularity Allocations To: Ard Biesheuvel , devel@edk2.groups.io Cc: Leif Lindholm , Sami Mujawar , Liming Gao References: <20240215003412.30983-1-osde@linux.microsoft.com> From: "Oliver Smith-Denny" In-Reply-To: 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: eQyZO9ihm6ienH8SUGLYAHHzx7686176AA= 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=Nf3RgP+j; 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 2/14/2024 11:50 PM, Ard Biesheuvel wrote> On Thu, 15 Feb 2024 at=20 01:34, Oliver Smith-Denny > wrote >> This could also be fixed with rearchi= tecting the heap guard system to >> respect alignment requirements and shift the guard pages inside of the >> outer rounded allocation or by having guard pages be the runtime >> granularity. Both of these approaches have issues, in the former, the >> allocator of runtime memory would get an address that was not aligned >> with the runtime granularity (the head guard would be, but not the >> usuable address), which seems fraught with peril. >=20 > This would be my preference, and I wouldn't expect huge problems with > code expecting a certain alignment for such allocations. The 64k > requirement is entirely to ensure that the OS does not have to guess > how it should map 64k pages that have conflicting memory attributes > due to being covered by two different misaligned entries in the UEFI > memory map. >=20 > This is also why this is important for the MAT and runtime services > code/data regions: without 64k alignment, there will be a piece in the > middle of each runtime DXE that requires both write and execute > permissions. >=20 >=20 I do have a PR up to fix the misalignment bug (I was doing a CI check on it before sending the patch when I did some further testing to discover the guard pages pushing out the allocation size). Would you prefer that I update that to do the guard page allocation inside the 64k allocation? I can certainly do that, again my concern is that the code starts getting more complex, with more room for errors. The heap guard code now needs to know the actual size requested by the caller, not the rounded size, so we can see, oh, the allocation requested is 64k, so I need another 64k region to fit the guards into, unless we already have shared guard pages, in which case we may not need to, or one guard may be shared and the other isn't, etc. It is doable, but I worry about the added complexity for only a small window of protection for these runtime memory regions. We could also say no shared guard pages for runtime regions if you don't have runtime allocation granularity equal to the EFI_PAGE_SIZE. Based on our offline conversation, I thought you were ok with the simple approach of disable the guards for these regions, the value of protecting these regions at boot time is not worth the additional complexity. But, I can update my PR to put the guards inside the allocation and we can compare the relative complexity. >> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.d= ec >> index a2cd83345f5b..884734aff592 100644 >> --- a/MdeModulePkg/MdeModulePkg.dec >> +++ b/MdeModulePkg/MdeModulePkg.dec >> @@ -1027,6 +1027,11 @@ [PcdsFixedAtBuild] >> # free pages for all of them. The page allocation for the type relat= ed to >> # cleared bits keeps the same as ususal. >> # >> + # The heap guard system only supports guarding EfiRuntimeServicesCode= , EfiRuntimeServicesData, >> + # EfiACPIReclaimMemory, and EfiACPIMemoryNVS memory types for systems= that have >=20 > I looked at the EFI spec again, and EfiACPIReclaimMemory is not > actually listed as a memory type that has this 64k alignment > requirement. This makes sense, given that this memory type has no > significance to the firmware itself, only to the OS. OTOH, reserved > memory does appear there. >=20 > So I suggest we fix that first, and then drop any mention of > EfiACPIReclaimMemory from this patch. At least we'll have heap guard > coverage for ACPI table allocations on arm64 going forward. >=20 > The logic in question was added in 2007 in commit 28a00297189c, so > this was probably the rule on Itanium, but that support is long gone. >=20 Thanks for looking this up. I'll update either this patch or the unsent patch I have depending on the direction we go. Thanks, Oliver -=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 (#115528): https://edk2.groups.io/g/devel/message/115528 Mute This Topic: https://groups.io/mt/104364784/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-