From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web11.10375.1600856856165654309 for ; Wed, 23 Sep 2020 03:27:36 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=EGfu6npT; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1600856855; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=xOHU+7h8psPFEyzAxVfmgmNEyhPvsTC2ps80C4wH9pU=; b=EGfu6npTLmtnwLPeXJfa8uVC9mFSX7m+RnPpOjyz05DHfp9azrpmCAtfmQXYJgWva04qzW F5GWTyz25qgwb1rS06o94GJizkYBUKe6uS6MPWkxJB6JJaSCLiEM8hgzky0yb08hScS4ee 2hVMpRF+st2lKbZtQV9foPN/jCtYBTg= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-448-igM7yLcXMNG1VMEY_wpT5Q-1; Wed, 23 Sep 2020 06:27:26 -0400 X-MC-Unique: igM7yLcXMNG1VMEY_wpT5Q-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 9D128805EE5; Wed, 23 Sep 2020 10:27:23 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-233.ams2.redhat.com [10.36.112.233]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6688778823; Wed, 23 Sep 2020 10:27:21 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/Mem: Memory paging related attributes must be separated from others To: devel@edk2.groups.io, jacek.kukiello@intel.com References: <20200923080829.6884-1-jacek.kukiello@intel.com> Cc: Jian J Wang , Hao A Wu , Dandan Bi , Liming Gao , Oleksiy Yakovlev , "Ard Biesheuvel (ARM address)" From: "Laszlo Ersek" Message-ID: Date: Wed, 23 Sep 2020 12:27:20 +0200 MIME-Version: 1.0 In-Reply-To: <20200923080829.6884-1-jacek.kukiello@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US On 09/23/20 10:08, Malgorzata Kukiello wrote: > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2982 > > During a recent change (bda18f5ed04f5317ab2bdc9da4530c078b33cc63) that changed > the way memory attributes are handled it affected a function in Page.c that > would clear all memory attributes even security related ones > > Signed-off-by: Malgorzata Kukiello > --- > MdeModulePkg/Core/Dxe/Mem/Page.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c > index 2c2c9cd6c3..9254afb92b 100644 > --- a/MdeModulePkg/Core/Dxe/Mem/Page.c > +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c > @@ -1933,7 +1933,7 @@ CoreGetMemoryMap ( > MemoryMapEnd = MemoryMap; > MemoryMap = MemoryMapStart; > while (MemoryMap < MemoryMapEnd) { > - MemoryMap->Attribute &= ~(UINT64)EFI_MEMORY_ATTRIBUTE_MASK; > + MemoryMap->Attribute &= ~(UINT64)EFI_MEMORY_ACCESS_MASK; > MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap, Size); > } > MergeMemoryMap (MemoryMapStart, &BufferSize, Size); > (1) Please CC the maintainers that need to review this patch. I'm doing that for you, for now. $ python BaseTools/Scripts/GetMaintainer.py -l \ MdeModulePkg/Core/Dxe/Mem/Page.c (But note that "BaseTools/Scripts/GetMaintainer.py" can operate on patches / commits as well; it's better to use it that way, and then to add corresponding "Cc:" tags to the commit messages themselves.) (2) Also CC'ing Oleksiy (author of the commit in question), and Ard (Linux EFI subsystem maintainer). (3) Your commit reference in the commit message is *still* wrong (I corrected it before in the BZ). The proper (upstream) commit hash is 3bd5c994c879f78e8e3d5346dc3b627f199291aa. (4) This patch should be patch#2 in a series of two patches. Currently this patch has been posted as a thread starter, and the more foundational patch (the one that modifies the macros in "UefiSpec.h") has been posted in response to this one. Instead, there should be a cover letter 0/2, then 1/2 (in response to the cover letter) should modify the macros, then finally this patch should be 2/2. Please run "BaseTools/Scripts/SetupGit.py" in your edk2 clone; it will set up shallow threading (meaning sendemail.thread=True AND sendemail.chainreplyto=False). (5) A better subject line for this patch, in my opinion, would be: MdeModulePkg/Core/Dxe: expose SP and CRYPTO capabilities in UEFI memmap Because, near the location that you are modifying, we have the following helpful comment: // // Note: Some OSs will treat EFI_MEMORY_DESCRIPTOR.Attribute as really // set attributes and change memory paging attribute accordingly. // But current EFI_MEMORY_DESCRIPTOR.Attribute is assigned by // value from Capabilities in GCD memory map. This might cause // boot problems. Clearing all paging related capabilities can // workaround it. Following code is supposed to be removed once // the usage of EFI_MEMORY_DESCRIPTOR.Attribute is clarified in // UEFI spec and adopted by both EDK-II Core and all supported // OSs. // Which in fact clarifies your issue report: apparently, the SP and CRYPTO attributes *are* already treated as capabilities (and not as presently set attributes) by operating systems. Therefore we should not hide these capabilities from OSes -- we should not allow the workaround to cover these capabilities. (BTW, when Oleksiy posted the original patch and we reviewed it, I believe this information -- i.e. how OSes would treat these new capabilities -- was not at our disposal. So I think it was impossible to tell whether we should hide SP and CRYPTO too, or not.) (6) I think the comment I quoted above should be clarified too. It currently says "Clearing all paging related capabilities". It should say "Clearing all page-access permission capabilities", or something similar; to better reflect the purpose of EFI_MEMORY_ACCESS_MASK. Thanks Laszlo