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

["Yao, Jiewen" <jiewen.yao@intel.com>]

I second.

Since this patch breaks existing OS, I agree that we should rollback the memory map change.
It means we can use original memory map, by filtering all page attributes.
I also suggest we add detail comment on why we do that.
GCD map is OK, which has both attribute and capability. And GCD is OS invisible.

For the ambiguity of the UEFI spec, I also agree. Maybe USWG is a better place to clarify the definition. But that cannot resolve the compatibility of exiting UEFI OS.

Since we have already seen 2 regression and compatibility issues related to UEFI memory map change, I suggest we always keep UEFI memory map always unchanged at first, unless we validated all production UEFI OS, to make sure no regression at all.

Thank you
Yao Jiewen


> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, November 16, 2017 10:47 AM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek
> <lersek@redhat.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Matt Fleming
> <matt@codeblueprint.co.uk>; edk2-devel@lists.01.org; Yao, Jiewen
> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: RE: [PATCH v5] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in
> memory map
> 
> Seemingly, the Memory Attributes Table should be consumed for memory
> protection.
> 
> UEFI spec: "The Memory Attributes Table is currently used to describe memory
> protections that may be applied to the EFI Runtime code and data by an
> operating system or hypervisor."
> Someone (Jiewen?) familiar with the table could help introduce the background.
> 
> And seemingly, the capabilities in UEFI memory map could not have paging
> related as it will break the compatibility to OS.
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Wednesday, November 15, 2017 11:59 PM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>;
> Matt Fleming <matt@codeblueprint.co.uk>; edk2-devel@lists.01.org; Yao,
> Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: Re: [PATCH v5] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in
> memory map
> 
> On 15 November 2017 at 15:48, Laszlo Ersek <lersek@redhat.com> wrote:
> > Hi Jian,
> >
> > On 11/15/17 10:27, Wang, Jian J wrote:
> >> I tried this workaround and there're no failure in booting Fedora 26
> >> and Windows server 2016 now. If no objection, I'll merge it into new version of
> this patch.
> >
> > I'm not too familiar with the Linux kernel's EFI pieces myself; that's
> > why I added Ard and Matt earlier to the thread (when I responded with
> > the regression report) -- Ard and Matt maintain EFI in the Linux kernel.
> >
> > So, if you think there's a bug in Linux (i.e., a mis-interpretation of
> > the UEFI spec), can you guys please discuss that together? Below you wrote:
> >
> > - "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"
> >
> 
> The UEFI spec does not distinguish between capabilities and attributes, so how
> on earth should the OS be able to make this distinction?
> 
> For instance, the UEFI spec defines EFI_MEMORY_RO as
> 
> """
> Physical memory protection attribute: The memory region supports making this
> memory range read-only by system hardware.
> """
> 
> which is essentially a capability not an attribute. However, EFI_MEMORY_RO/XP
> are also used to convey information about the nature of the *contents* of the
> memory region, i.e., is it .text, .rodata or .data/.bss
> 
> So given that the OS only has the UEFI memory map to go on, how exactly should
> it figure out what these attributes are meant to imply?
> 
> > - "there's real gap between UEFI and OS on how to interpret the memory
> >    capabilities"
> >
> 
> Yes. There is also a gap between how GCD and the UEFI memory map interpret
> these attributes, which is why nobody bothers to set the protection capabilities
> for GCD: it gets copied into the UEFI memory map, and nobody has a clue how
> the OS should treat it.
> 
> When the [short lived]
> EFI_PROPERTIES_RUNTIME_MEMORY_PROTECTION_NON_EXECUTABLE_PE_DA
> TA
> feature was introduced, it was essentially decided that RO and XP may be used to
> annotate the nature of the contents of memory regions, where code was
> mapped RO and data mapped XP.
> 
> > Why is it wrong for the OS kernel, according to the UEFI spec, to
> > change the attributes / mappings of a memory area, as long as it stays
> > compliant with the advertised capabilities? How is an OS expected to
> > work, upon seeing those new "paging capabilities from UEFI memory map"
> > (in Star's words)?
> >
> > Apparently, it's not just Linux that's confused; see the Win2016 crash
> > too. Is the UEFI spec detailed enough on those capabilities? (Recently
> > I tried to review the paging capabilities myself in the spec, and I
> > ended up totally confused...)
> >
> 
> I think it is simply impossible at this point to interpret those flags as attributes, i.e.,
> RO means code that may be mapped read-only, and XP means data that may be
> mapped non-executable. Anything beyond that opens a can of worms that is
> bound to break stuff all over the place.
> 
> --
> Ard.