From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail04.groups.io (mail04.groups.io [45.79.224.9]) by spool.mail.gandi.net (Postfix) with ESMTPS id DFA827803E6 for ; Wed, 17 Apr 2024 06:38:58 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=0XTpgKnHaXYQVUrxGUMTq09MN/85XYAN3fIgh+SFxVI=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Resent-Date:Resent-From:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20240206; t=1713335937; v=1; b=B/enQcissDDWaCLfhRsIe+Cl4K7wNU+BmEbrJW+8eouTRtUTwa14B09jU6qYxLB2h7017/Kf IHZno0FkPs6zkKSE7YXkLvb96SeeuUoo7hk2ekmt0XdD/JJdcS5SIgJZLzt2Brif65a3Gvl1gWB a2pMeklEMM/ZGPkNhs9yWZgSqtFlzFcdCkRZHhHuH7sA0RQ0i2vNunX3Y7tbsb2/Ci2yp0T2vke RWsWugp4K6zkUe894jtCV8NuEv5Px1ZM8YYCYzImLYkIGbId5mnA6gQ3g1s6HdZOyI4wZ4Q3KQA foNMyK3pR3of0wKQebSbv/5gGkfPlf151EjqclMWgR+Hg== X-Received: by 127.0.0.2 with SMTP id MrykYY7687511xJ56v5SfxWX; Tue, 16 Apr 2024 23:38:57 -0700 X-Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web10.6638.1713335936741970229 for ; Tue, 16 Apr 2024 23:38:56 -0700 X-Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 262076136F for ; Wed, 17 Apr 2024 06:38:56 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id C9B1BC32783 for ; Wed, 17 Apr 2024 06:38:55 +0000 (UTC) X-Received: by mail-lj1-f170.google.com with SMTP id 38308e7fff4ca-2db2f6cb312so3016991fa.2 for ; Tue, 16 Apr 2024 23:38:55 -0700 (PDT) X-Gm-Message-State: CLwDyTZEC1EGbLBEklqNqxdgx7686176AA= X-Google-Smtp-Source: AGHT+IEa4Rj1fEKbTnZL/gsdEQiwXoHIOTnYwzYv+BVKLT3EIX0NaZPADGeLGc+2Ir2/yDTaSNUxLQPHMjWolyVoPRw= X-Received: by 2002:a2e:a483:0:b0:2d8:68ef:9454 with SMTP id h3-20020a2ea483000000b002d868ef9454mr11869772lji.47.1713335933897; Tue, 16 Apr 2024 23:38:53 -0700 (PDT) MIME-Version: 1.0 References: <20240417022836.1593-1-taylor.d.beebe@gmail.com> In-Reply-To: <20240417022836.1593-1-taylor.d.beebe@gmail.com> From: "Ard Biesheuvel" Date: Wed, 17 Apr 2024 08:38:42 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH v1] MdeModulePkg: Fixup MAT Attributes After Splitting EFI Memory Map To: devel@edk2.groups.io, taylor.d.beebe@gmail.com Cc: Liming Gao 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 Resent-Date: Tue, 16 Apr 2024 23:38:56 -0700 Resent-From: ardb@kernel.org Reply-To: devel@edk2.groups.io,ardb@kernel.org List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240206 header.b="B/enQcis"; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=kernel.org (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 45.79.224.9 as permitted sender) smtp.mailfrom=bounce@groups.io Hi Taylor, On Wed, 17 Apr 2024 at 04:28, Taylor Beebe wrote: > > The Memory Attributes Table is generated by fetching the EFI > memory map and splitting entries which contain loaded > images so DATA and CODE sections have separate descriptors. > The splitting is done via a call to SplitTable() which > marks image DATA sections with the EFI_MEMORY_XP attribute > and CODE sections with the EFI_MEMORY_RO attribute when > splitting. After this process, there may still be > EfiRuntimeServicesCode regions which did not have their > attributes set because they are not part of loaded images. > > This patch updates the MAT EnforceMemoryMapAttribute logic > to set the access attributes of runtime memory regions > which are not part of loaded images (have not had their > access attributes set). > > Cc: Liming Gao > Signed-off-by: Taylor Beebe > --- > MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c | 29 ++++++++++---------- > 1 file changed, 15 insertions(+), 14 deletions(-) > > diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c > index e9343a2c4e..1d9f935db0 100644 > --- a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c > +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c > @@ -425,8 +425,8 @@ MergeMemoryMap ( > } > > /** > - Enforce memory map attributes. > - This function will set EfiRuntimeServicesData/EfiMemoryMappedIO/EfiMemoryMappedIOPortSpace to be EFI_MEMORY_XP. > + Walk the memory map and set EfiRuntimeServicesData/EfiMemoryMappedIO/EfiMemoryMappedIOPortSpace > + to EFI_MEMORY_XP and EfiRuntimeServicesCode to EFI_MEMORY_RO. > > @param MemoryMap A pointer to the buffer in which firmware places > the current memory map. > @@ -447,18 +447,19 @@ EnforceMemoryMapAttribute ( > MemoryMapEntry = MemoryMap; > MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *)((UINT8 *)MemoryMap + MemoryMapSize); > while ((UINTN)MemoryMapEntry < (UINTN)MemoryMapEnd) { > - switch (MemoryMapEntry->Type) { > - case EfiRuntimeServicesCode: > - // do nothing > - break; > - case EfiRuntimeServicesData: > - case EfiMemoryMappedIO: > - case EfiMemoryMappedIOPortSpace: > - MemoryMapEntry->Attribute |= EFI_MEMORY_XP; > - break; > - case EfiReservedMemoryType: > - case EfiACPIMemoryNVS: > - break; > + if ((MemoryMapEntry->Attribute & EFI_MEMORY_ACCESS_MASK) == 0) { > + switch (MemoryMapEntry->Type) { > + case EfiRuntimeServicesCode: > + MemoryMapEntry->Attribute |= EFI_MEMORY_RO; I'm not sure this is safe. If EFI_MEMORY_RO is not set, it might be intentional. Note that the purpose of the MAT is primarily to describe permission attributes that are more granular than the entry itself. Originally, we tried to split those in the memory map but that broke SetVirtualAddressMap() so we were forced to add a second table instead. For entries where we lack such additional metadata, I don't think we can make assumptions based on the type beyond mapping data and MMIO regions XP. We have no idea how those EfiRuntimeServicesCode regions may be used, and currently, the spec permits RWX semantics for those if no restrictions are specified. The spec suggests that omitting an entry from the MAT is the same as listing it with RO|XP cleared. How RO|XP from the original entry should be interpreted wrt to the MAT is unspecified. So I think the only thing we can do at this point is preserve the entry if it has RO|XP set, or just drop it otherwise. Note that the spec also mentions that the MAT must only contain EfiRuntimeServicesCode or EfiRuntimeServicesData entries, and it looks like this is not being enforced either. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117895): https://edk2.groups.io/g/devel/message/117895 Mute This Topic: https://groups.io/mt/105570114/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-