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

["Wang, Jian J" <jian.j.wang@intel.com>]

Hi Laszlo,

I did some investigation works on this issue. I think I found the root cause and
tend to believe this is a Fedora kernel issue. Here're proves:

memmap output patterns in which Fedora 26 failed to boot:
1) BS_Data    00000000AE200000-00000000AE20EFFF 000000000000000F 000000000002600F
    RT_Data    00000000AE20F000-00000000AE38EFFF 0000000000000180 800000000002600F
    RT_Code    00000000AE38F000-00000000AE48EFFF 0000000000000100 800000000002600F
    Reserved   00000000AE48F000-00000000AE58EFFF 0000000000000100 000000000002600F

2) BS_Data    00000000AE200000-00000000AE20EFFF 000000000000000F 000000000000000F
    RT_Data    00000000AE20F000-00000000AE38EFFF 0000000000000180 800000000000000F
    RT_Code    00000000AE38F000-00000000AE467FFF 00000000000000D9 800000000000000F
    RT_Code    00000000AE468000-00000000AE469FFF 0000000000000002 800000000002600F
    RT_Code    00000000AE46A000-00000000AE46EFFF 0000000000000005 800000000000000F
    RT_Code    00000000AE46F000-00000000AE470FFF 0000000000000002 800000000002600F
    RT_Code    00000000AE471000-00000000AE475FFF 0000000000000005 800000000000000F
    RT_Code    00000000AE476000-00000000AE479FFF 0000000000000004 800000000002600F
    RT_Code    00000000AE47A000-00000000AE47FFFF 0000000000000006 800000000000000F
    RT_Code    00000000AE480000-00000000AE481FFF 0000000000000002 800000000002600F
    RT_Code    00000000AE482000-00000000AE487FFF 0000000000000006 800000000000000F
    RT_Code    00000000AE488000-00000000AE489FFF 0000000000000002 800000000002600F
    RT_Code    00000000AE48A000-00000000AE48EFFF 0000000000000005 800000000000000F
    Reserved   00000000AE48F000-00000000AE58EFFF 0000000000000100 000000000000000F

3) RT_Data    00000000AE20F000-00000000AE38EFFF 0000000000000180 800000000000000F
    RT_Code    00000000AE38F000-00000000AE418FFF 000000000000008A 800000000000000F
    RT_Code    00000000AE419000-00000000AE48EFFF 0000000000000076 800000000002600F
    Reserved   00000000AE48F000-00000000AE58EFFF 0000000000000100 000000000000000F

4) BS_Data    00000000AE200000-00000000AE20EFFF 000000000000000F 000000000002400F
    RT_Data    00000000AE20F000-00000000AE38EFFF 0000000000000180 800000000002400F
    RT_Code    00000000AE38F000-00000000AE48EFFF 0000000000000100 800000000002400F
    Reserved   00000000AE48F000-00000000AE58EFFF 0000000000000100 000000000002400F


memmap output pattern in which Fedora 26 booted successfully
5) BS_Data    00000000AE200000-00000000AE20EFFF 000000000000000F 000000000000000F
    RT_Data    00000000AE20F000-00000000AE38EFFF 0000000000000180 800000000000000F
    RT_Code    00000000AE38F000-00000000AE48EFFF 0000000000000100 800000000000000F
    Reserved   00000000AE48F000-00000000AE58EFFF 0000000000000100 000000000000000F

6) BS_Data    00000000AE200000-00000000AE20EFFF 000000000000000F 000000000000000F
    RT_Data    00000000AE20F000-00000000AE38EFFF 0000000000000180 800000000000000F
    RT_Code    00000000AE38F000-00000000AE418FFF 000000000000008A 800000000000000F
    RT_Code    00000000AE419000-00000000AE419FFF 0000000000000001 800000000000400F
    RT_Code    00000000AE41A000-00000000AE41BFFF 0000000000000002 800000000002000F
    RT_Code    00000000AE41C000-00000000AE420FFF 0000000000000005 800000000000400F
    RT_Code    00000000AE421000-00000000AE422FFF 0000000000000002 800000000002000F
    RT_Code    00000000AE423000-00000000AE427FFF 0000000000000005 800000000000400F
    RT_Code    00000000AE428000-00000000AE42AFFF 0000000000000003 800000000002000F
    RT_Code    00000000AE42B000-00000000AE42FFFF 0000000000000005 800000000000400F
    RT_Code    00000000AE430000-00000000AE432FFF 0000000000000003 800000000002000F
    RT_Code    00000000AE433000-00000000AE439FFF 0000000000000007 800000000000400F
    RT_Code    00000000AE43A000-00000000AE43DFFF 0000000000000004 800000000002000F
    RT_Code    00000000AE43E000-00000000AE444FFF 0000000000000007 800000000000400F
    RT_Code    00000000AE445000-00000000AE448FFF 0000000000000004 800000000002000F
    RT_Code    00000000AE449000-00000000AE44EFFF 0000000000000006 800000000000400F
    RT_Code    00000000AE44F000-00000000AE450FFF 0000000000000002 800000000002000F
    RT_Code    00000000AE451000-00000000AE455FFF 0000000000000005 800000000000400F
    RT_Code    00000000AE456000-00000000AE458FFF 0000000000000003 800000000002000F
    RT_Code    00000000AE459000-00000000AE45FFFF 0000000000000007 800000000000400F
    RT_Code    00000000AE460000-00000000AE461FFF 0000000000000002 800000000002000F
    RT_Code    00000000AE462000-00000000AE467FFF 0000000000000006 800000000000400F
    RT_Code    00000000AE468000-00000000AE469FFF 0000000000000002 800000000002000F
    RT_Code    00000000AE46A000-00000000AE46EFFF 0000000000000005 800000000000400F
    RT_Code    00000000AE46F000-00000000AE470FFF 0000000000000002 800000000002000F
    RT_Code    00000000AE471000-00000000AE475FFF 0000000000000005 800000000000400F
    RT_Code    00000000AE476000-00000000AE479FFF 0000000000000004 800000000002000F
    RT_Code    00000000AE47A000-00000000AE47FFFF 0000000000000006 800000000000400F
    RT_Code    00000000AE480000-00000000AE481FFF 0000000000000002 800000000002000F
    RT_Code    00000000AE482000-00000000AE487FFF 0000000000000006 800000000000400F
    RT_Code    00000000AE488000-00000000AE489FFF 0000000000000002 800000000002000F
    RT_Code    00000000AE48A000-00000000AE48EFFF 0000000000000005 800000000000400F
    Reserved   00000000AE48F000-00000000AE58EFFF 0000000000000100 000000000000000F

You may notice that 4) will fail but 6) will succeed. Taking into account the fact that failure
always happened in the runtime service api (set_variable), I think it must be that the kernel will
mark the memory block to be RO/XP/RP memory according to its capabilities but not its current
attributes. 

This can explain why 4) will fail but 6) will succeed. Although memmap shows all runtime 
driver image memory as RT_Code, but they're not all code segment. Instead some of them are 
actually data segment.

It's normal to mark code segment to be RO and data segment to be XP. But mark data segment
to be RO will cause runtime services failure. 4) shows all RT_Code to be XXX24XXX, which means
Fedora kernel will mark all code segment and data segment to be RO and XP mistakenly, based 
on my previous hypothesis. This can also explain why 1), 2), 3) will fail the boot because XXX26XXX
will let Fedora kernel to mark RT_Code memory block to be not-present.

I haven't got time to look into the Linux kernel source so I can't confirm above analysis yet.
I think you're more familiar with kernel source than us. Maybe you could help to take a look.

Thanks,
Jian

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, November 10, 2017 8:24 PM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Matt
> Fleming <matt@codeblueprint.co.uk>
> Subject: Re: [PATCH v5] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in
> memory map
> 
> Hi Jian,
> 
> I'm CC'ing Ard and Matt, and commenting at the bottom.
> 
> On 11/10/17 02:02, Jian J Wang wrote:
> >> v5:
> >>    Coding style clean-up
> >
> >> v4:
> >> a. Remove DoUpdate and check attributes mismatch all the time to avoid
> >>    a logic hole
> >> b. Add warning message if failed to update capability
> >> c. Add local variable to hold new attributes to make code cleaner
> >
> >> v3:
> >> a. Add comment to explain more on updating memory capabilities
> >> b. Fix logic hole in updating attributes
> >> c. Instead of checking illegal memory space address and size, use return
> >>    status of gDS->SetMemorySpaceCapabilities() to skip memory block which
> >>    cannot be updated with new capabilities.
> >
> >> v2
> >> a. Fix an issue which will cause setting capability failure if size is smaller
> >>    than a page.
> >
> > 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.
> >
> > More detailed information, please refer to
> >     https://bugzilla.tianocore.org/show_bug.cgi?id=753
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> >  UefiCpuPkg/CpuDxe/CpuPageTable.c | 69
> +++++++++++++++++++++++++++++-----------
> >  1 file changed, 50 insertions(+), 19 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > index d312eb66f8..61537838b7 100644
> > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > @@ -789,8 +789,7 @@ RefreshGcdMemoryAttributesFromPaging (
> >    UINT64                              BaseAddress;
> >    UINT64                              PageStartAddress;
> >    UINT64                              Attributes;
> > -  UINT64                              Capabilities;
> > -  BOOLEAN                             DoUpdate;
> > +  UINT64                              NewAttributes;
> >    UINTN                               Index;
> >
> >    //
> > @@ -802,9 +801,8 @@ RefreshGcdMemoryAttributesFromPaging (
> >
> >    GetCurrentPagingContext (&PagingContext);
> >
> > -  DoUpdate      = FALSE;
> > -  Capabilities  = 0;
> >    Attributes    = 0;
> > +  NewAttributes = 0;
> >    BaseAddress   = 0;
> >    PageLength    = 0;
> >
> > @@ -813,6 +811,34 @@ RefreshGcdMemoryAttributesFromPaging (
> >        continue;
> >      }
> >
> > +    //
> > +    // Sync the actual paging related capabilities back to GCD service first.
> > +    // As a side effect (good one), this can also help to avoid unnecessary
> > +    // memory map entries due to the different capabilities of the same type
> > +    // memory, such as multiple RT_CODE and RT_DATA entries in memory
> map,
> > +    // which could cause boot failure of some old Linux distro (before v4.3).
> > +    //
> > +    Status = gDS->SetMemorySpaceCapabilities (
> > +                    MemorySpaceMap[Index].BaseAddress,
> > +                    MemorySpaceMap[Index].Length,
> > +                    MemorySpaceMap[Index].Capabilities |
> > +                    EFI_MEMORY_PAGETYPE_MASK
> > +                    );
> > +    if (EFI_ERROR (Status)) {
> > +      //
> > +      // If we cannot udpate the capabilities, we cannot update its
> > +      // attributes either. So just simply skip current block of memory.
> > +      //
> > +      DEBUG ((
> > +        DEBUG_WARN,
> > +        "Failed to update capability: [%lu] %016lx - %016lx (%016lx -
> > %016lx)\r\n",
> > +        (UINT64)Index, BaseAddress, BaseAddress + Length - 1,
> > +        MemorySpaceMap[Index].Capabilities,
> > +        MemorySpaceMap[Index].Capabilities | EFI_MEMORY_PAGETYPE_MASK
> > +        ));
> > +      continue;
> > +    }
> > +
> >      if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
> >        //
> >        // Current memory space starts at a new page. Resetting PageLength will
> > @@ -826,7 +852,9 @@ RefreshGcdMemoryAttributesFromPaging (
> >        PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
> >      }
> >
> > -    // Sync real page attributes to GCD
> > +    //
> > +    // Sync actual page attributes to GCD
> > +    //
> >      BaseAddress       = MemorySpaceMap[Index].BaseAddress;
> >      MemorySpaceLength = MemorySpaceMap[Index].Length;
> >      while (MemorySpaceLength > 0) {
> > @@ -842,23 +870,26 @@ RefreshGcdMemoryAttributesFromPaging (
> >          PageStartAddress  = (*PageEntry) &
> (UINT64)PageAttributeToMask(PageAttribute);
> >          PageLength        = PageAttributeToLength (PageAttribute) - (BaseAddress -
> PageStartAddress);
> >          Attributes        = GetAttributesFromPageEntry (PageEntry);
> > -
> > -        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;
> > -        }
> >        }
> >
> >        Length = MIN (PageLength, MemorySpaceLength);
> > -      if (DoUpdate) {
> > -        gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
> > -        gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> > -        DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx
> - %016lx (%08lx -> %08lx)\r\n",
> > -                             Index, BaseAddress, BaseAddress + Length - 1,
> > -                             MemorySpaceMap[Index].Attributes, Attributes));
> > +      if (Attributes != (MemorySpaceMap[Index].Attributes &
> > +                         EFI_MEMORY_PAGETYPE_MASK)) {
> > +        NewAttributes = (MemorySpaceMap[Index].Attributes &
> > +                         ~EFI_MEMORY_PAGETYPE_MASK) | Attributes;
> > +        Status = gDS->SetMemorySpaceAttributes (
> > +                        BaseAddress,
> > +                        Length,
> > +                        NewAttributes
> > +                        );
> > +        ASSERT_EFI_ERROR (Status);
> > +        DEBUG ((
> > +          DEBUG_INFO,
> > +          "Updated memory space attribute: [%lu] %016lx - %016lx (%016lx -
> > %016lx)\r\n",
> > +          (UINT64)Index, BaseAddress, BaseAddress + Length - 1,
> > +          MemorySpaceMap[Index].Attributes,
> > +          NewAttributes
> > +          ));
> >        }
> >
> >        PageLength        -= Length;
> >
> 
> So, I was ready to give my R-b for this patch, but then I also wanted to
> test it. I applied the patch on current edk2 master (7e2a8dfe8a9a,
> "ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI
> core", 2017-10-20), and built OVMF like this:
> 
> $ build \
>   -a IA32 \
>   -a X64 \
>   -p OvmfPkg/OvmfPkgIa32X64.dsc \
>   -t GCC48 \
>   -b NOOPT \
>   -D SMM_REQUIRE \
>   -D SECURE_BOOT_ENABLE \
>   -D E1000_ENABLE \
>   -D HTTP_BOOT_ENABLE
> 
> For testing I used a recent-ish upstream QEMU development build
> (ae49fbbcd8e4, "Merge remote-tracking branch
> 'remotes/rth/tags/pull-tcg-20171025' into staging", 2017-10-25), with
> the Q35 machine type (which is required by SMM anyway).
> 
> The results vary across guest OSes:
> 
> (1) Up-to-date Fedora 26 guest crashes during boot, with the following
> call stack:
> 
> BUG: unable to handle kernel paging request at fffffffefe893018
> Call Trace:
>  ? __change_page_attr_set_clr+0xaa6/0xd70
>  ? kernel_map_pages_in_pgd+0xbc/0xd0
>  ? efi_call+0x58/0x90
>  ? virt_efi_set_variable.part.7+0x66/0x120
>  ? virt_efi_set_variable+0x4f/0x60
>  ? efi_delete_dummy_variable+0x62/0x90
>  ? efi_enter_virtual_mode+0x4d4/0x4e8
>  ? efi_enter_virtual_mode+0x4d4/0x4e8
>  ? start_kernel+0x442/0x4e6
>  ? early_idt_handler_array+0x120/0x120
>  ? x86_64_start_reservations+0x24/0x26
>  ? x86_64_start_kernel+0x13e/0x161
>  ? secondary_startup_64+0x9f/0x9f
> 
> (2) The following Windows OSes all boot successfully:
> 
> - Windows 7
> - Windows Server 2008 R2
> - Windows 8.1
> - Windows Server 2012 R2
> - Windows 10
> 
> (3) Windows Server 2016 crashes with a BSOD; reporting "ATTEMPTED WRITE
> TO READONLY MEMORY".
> 
> (Without the patch, all OSes boot OK.)
> 
> 
> I'm attaching a ZIP file with the following contents (note that I'll
> attach the same file to TianoCore BZ#753 as well, because the mailing
> list archive(s) don't seem to preserve attachments):
> 
> - "ovmf.pre.txt", "shell.memmap.pre.txt", "kernel.pre.txt": OVMF log,
> MEMMAP command output in the UEFI shell, and Fedora 26 kernel boot log
> (successful) *before* applying your patch. The kernel log is detailed
> (the cmdline had "ignore_loglevel" and "efi=debug").
> 
> - "ovmf.post.txt", "shell.memmap.post.txt", "kernel.post.txt": same
> files as above, but saved *after* applying your patch. This is when the
> F26 kernel crashes.
> 
> - "win2016.post.png": screenshot of the Windows Server 2016 boot failure
> (after the patch was applied).
> 
> Thanks,
> Laszlo