From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2602C202E5CD3 for ; Thu, 26 Oct 2017 01:38:47 -0700 (PDT) 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 mx1.redhat.com (Postfix) with ESMTPS id 97E3E62E88; Thu, 26 Oct 2017 08:42:32 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 97E3E62E88 Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-110.rdu2.redhat.com [10.10.120.110]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3409B7F496; Thu, 26 Oct 2017 08:42:31 +0000 (UTC) To: "Wang, Jian J" , "edk2-devel@lists.01.org" Cc: "Yao, Jiewen" , "Dong, Eric" , Ard Biesheuvel References: <20171023065022.1272-1-jian.j.wang@intel.com> <764523da-0d75-d4ac-3121-c8282ada3e8f@redhat.com> <2367cd4c-d660-c848-acdf-c67fd94b4880@redhat.com> From: Laszlo Ersek Message-ID: Date: Thu, 26 Oct 2017 10:42:30 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Thu, 26 Oct 2017 08:42:32 +0000 (UTC) Subject: Re: [PATCH] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 26 Oct 2017 08:38:47 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/26/17 03:41, Wang, Jian J wrote: > Please see my comments below. Thanks. > >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Wednesday, October 25, 2017 8:51 PM >> To: Wang, Jian J ; edk2-devel@lists.01.org >> Cc: Yao, Jiewen ; Dong, Eric >> Subject: Re: [edk2] [PATCH] UefiCpuPkg/CpuDxe: Fix multiple entries of >> RT_CODE in memory map >> >> On 10/25/17 03:33, Wang, Jian J wrote: >>> Hi Laszlo, >>> >>> Thanks for the feedback. I'd like to explain a bit more here first and >>> will update the commit message later. >>> >>> The multiple RT_CODE entries issue was reported by LUV test suite from >>> https://01.org/linux-uefi-validation >>> >>> You're right this issue is caused by my commit c1cab54ce57c, which >>> tried to fix the GCD issue in which you can't set paging related >>> memory attributes through GCD service API. The reasons are that GCD >>> will filter out all paging related memory attributes and also, the CPU >>> driver didn't sync the paging attributes back to GCD. Sorry I don't >>> know why GCD and cpu driver are implemented that way. >>> >>> My previous commit c1cab54ce57c fixed above issues but didn't make >>> sure that all memory blocks share the same capabilities because I just >>> added paging related memory capabilities to those memory block having >>> some paging attributes set. This will in turn cause more than one >>> RT_CODE entries in memory map because GCD reports memory to OS based >>> on the memory block capability. >>> >>> Why multiple RT_CODE matters? It's because that Linux kernel would >>> misplace the runtime service code segment and its data segment. What I >>> know is this Linux issue had been fixed. That's why recent Linux >>> distro won't encounter problems even if we report multiple RT_CODE >>> memory to kernel. >> >> Ah, OK, I remember now. >> >> The point is that separate entries in the UEFI memmap may be mapped by >> the OS to discontiguous virtual address ranges. >> >> However, if we take the UEFI memmap entries that belong to a single >> runtime DXE driver, and unintentionally split those entries up (by >> setting distinct capabilities for a subset of their pages), then the >> runtime driver will break, because the linear address range that the >> driver expects (from its original loading and relocation) will not be >> kept linear by the OS. >> >>> I'm sorry that I cannot find the specific version of kernel which has >>> such problem and I can't find any discussion related. Maybe Jiewen can >>> provide more detailed information. >> >> It would be really helpful if you guys could name a guest kernel version >> (or a GNU/Linux distro release) that is affected by this problem. >> > > It seems following log history which mentioned the problem. > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=0ce3cc008ec04258b6a6314b09f1a6012810881a Ah, exactly. I vaguely remembered that the same issue had originally popped up in connection to the properties table. Commit 0ce3cc008ec0 ("arm64/efi: Fix boot crash by not padding between EFI_MEMORY_RUNTIME regions", 2015-09-25) was first released as part of Linux v4.3. > >> Are there perhaps any conditions that this issue depends on, such as >> PCDs for example? In other words, is it possible that various edk2 >> platform settings (in the DSC) can mask this issue? >> > > In current code base, following PCDs may cause memory page attribute changes > > PcdImageProtectionPolicy > PcdDxeNxMemoryProtectionPolicy > > Once the heap guard feature is checked in (you may notice my recent patches), > there're three more PCDs: > > PcdHeapGuardPropertyMask > PcdHeapGuardPoolType > PcdHeapGuardPageType > >> Can you maybe present UEFI memmaps (dumped in the UEFI shell, or in >> Linux) that show the problem, compared to this fix? >> > > Before this patch (after c1cab54ce57c), the memory map looks like > > RT_Data 00000000BE20F000-00000000BE38EFFF 0000000000000180 800000000000000F > RT_Code 00000000BE38F000-00000000BE46FFFF 00000000000000E1 800000000000000F > RT_Code 00000000BE470000-00000000BE470FFF 0000000000000001 800000000000400F > RT_Code 00000000BE471000-00000000BE472FFF 0000000000000002 800000000002000F > RT_Code 00000000BE473000-00000000BE476FFF 0000000000000004 800000000000400F > RT_Code 00000000BE477000-00000000BE478FFF 0000000000000002 800000000002000F > RT_Code 00000000BE479000-00000000BE47CFFF 0000000000000004 800000000000400F > RT_Code 00000000BE47D000-00000000BE47FFFF 0000000000000003 800000000002000F > RT_Code 00000000BE480000-00000000BE483FFF 0000000000000004 800000000000400F > RT_Code 00000000BE484000-00000000BE485FFF 0000000000000002 800000000002000F > RT_Code 00000000BE486000-00000000BE489FFF 0000000000000004 800000000000400F > RT_Code 00000000BE48A000-00000000BE48BFFF 0000000000000002 800000000002000F > RT_Code 00000000BE48C000-00000000BE48EFFF 0000000000000003 800000000000400F > > You may notice that there're one RT_Data with 12 RT_Code entries listed. > After this patch, it will be > > RT_Data 00000000BE20F000-00000000BE38EFFF 0000000000000180 800000000002600F > RT_Code 00000000BE38F000-00000000BE48EFFF 0000000000000100 800000000002600F > >>> This patch will make sure that all memory block share the same paging >>> capabilities. Because all memory are actually paged in current EDK2 >>> (at least IA processors), technically we're capable of setting any >>> page of memory to read-only and/or non-executable. I think this fix is >>> not only trying to avoid multiple RT_CODE memory map entries but also >>> trying to make sure the memory capabilities in GCD service to reflect >>> complete status of the real world. >> >> Are you saying that *any* firmware that carries commit c1cab54ce57c >> should also receive this patch? >> > > Yes. > >> If that's the case, then some kind of "reproducer" would be really nice >> -- steps that you can run both with and without this patch, and the >> output or the behavior will show the difference. >> > > PcdImageProtectionPolicy is enabled by default. It can be "naturally" reproduced. Thank you, Jian, your answers are very helpful! Laszlo >>>> -----Original Message----- >>>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>>> Sent: Tuesday, October 24, 2017 8:20 PM >>>> To: Wang, Jian J ; edk2-devel@lists.01.org >>>> Cc: Yao, Jiewen ; Dong, Eric >>>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/CpuDxe: Fix multiple entries of >>>> RT_CODE in memory map >>>> >>>> On 10/23/17 08:50, Jian J Wang wrote: >>>>> More than one entry of RT_CODE memory might cause boot problem for >>>> some >>>>> old OSs. This patch will fix this issue to keep OS compatibility as much >>>>> as possible. >>>>> >>>>> Cc: Eric Dong >>>>> Cc: Jiewen Yao >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>> Signed-off-by: Jian J Wang >>>>> --- >>>>> UefiCpuPkg/CpuDxe/CpuPageTable.c | 14 +++++++++++--- >>>>> 1 file changed, 11 insertions(+), 3 deletions(-) >>>> >>>> Can you please explain in the commit message; what OSes are affected by >>>> this issue, to your knowledge? >>>> >>>> Also, the code being patched seems to originate from commit c1cab54ce57c >>>> ("UefiCpuPkg/CpuDxe: Fix out-of-sync issue in page attributes", >>>> 2017-09-16). I vaguely recall that this commit was related to your "page >>>> 0 protection" work. >>>> >>>> Can you please explain, in the commit message, under what circumstances >>>> (PCD settings etc) the issue arises, and how we end up with multiple >>>> RT_CODE entries in the memory map? >>>> >>>> (BTW, multiple RT_CODE entries in the memmap should be perfectly >>>> normal... So I'm extra curious about the OSes that are not compatible >>>> with that.) >>>> >>>> Thanks, >>>> Laszlo >>>> >>>>> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c >>>> b/UefiCpuPkg/CpuDxe/CpuPageTable.c >>>>> index d312eb66f8..0802464b9d 100644 >>>>> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c >>>>> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c >>>>> @@ -829,6 +829,15 @@ RefreshGcdMemoryAttributesFromPaging ( >>>>> // Sync real page attributes to GCD >>>>> BaseAddress = MemorySpaceMap[Index].BaseAddress; >>>>> MemorySpaceLength = MemorySpaceMap[Index].Length; >>>>> + Capabilities = MemorySpaceMap[Index].Capabilities | >>>>> + EFI_MEMORY_PAGETYPE_MASK; >>>>> + Status = gDS->SetMemorySpaceCapabilities ( >>>>> + BaseAddress, >>>>> + MemorySpaceLength, >>>>> + Capabilities >>>>> + ); >>>>> + ASSERT_EFI_ERROR (Status); >>>>> + >>>>> while (MemorySpaceLength > 0) { >>>>> if (PageLength == 0) { >>>>> PageEntry = GetPageTableEntry (&PagingContext, BaseAddress, >>>> &PageAttribute); >>>>> @@ -846,7 +855,6 @@ RefreshGcdMemoryAttributesFromPaging ( >>>>> if (Attributes != (MemorySpaceMap[Index].Attributes & >>>> EFI_MEMORY_PAGETYPE_MASK)) { >>>>> DoUpdate = TRUE; >>>>> Attributes |= (MemorySpaceMap[Index].Attributes & >>>> ~EFI_MEMORY_PAGETYPE_MASK); >>>>> - Capabilities = Attributes | MemorySpaceMap[Index].Capabilities; >>>>> } else { >>>>> DoUpdate = FALSE; >>>>> } >>>>> @@ -854,8 +862,8 @@ RefreshGcdMemoryAttributesFromPaging ( >>>>> >>>>> Length = MIN (PageLength, MemorySpaceLength); >>>>> if (DoUpdate) { >>>>> - gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities); >>>>> - gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes); >>>>> + Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length, >>>> Attributes); >>>>> + ASSERT_EFI_ERROR (Status); >>>>> DEBUG ((DEBUG_INFO, "Update memory space attribute: >> [%02d] %016lx >>>> - %016lx (%08lx -> %08lx)\r\n", >>>>> Index, BaseAddress, BaseAddress + Length - 1, >>>>> MemorySpaceMap[Index].Attributes, Attributes)); >>>>> >>> >