Re: [PATCH] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

[Laszlo Ersek <lersek@redhat.com>]

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 <jian.j.wang@intel.com>; edk2-devel@lists.01.org
>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
>> 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 <jian.j.wang@intel.com>; edk2-devel@lists.01.org
>>>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
>>>> 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 <eric.dong@intel.com>
>>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
>>>>> ---
>>>>>  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));
>>>>>
>>>
>