Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled.

[Laszlo Ersek <lersek@redhat.com>]

On 09/14/17 11:39, Zeng, Star wrote:
> There is a realistic problem I just found and want to share.
> 
> GCD database is built based on resource hob reported from PEI, and
> GCD checks the input Attributes against Capabilities when setting
> Attributes, but we (I just searched the whole codebase) can see there
> is no code reporting resource hob with
> EFI_RESOURCE_ATTRIBUTE_READ_PROTECTABLE,
> EFI_RESOURCE_ATTRIBUTE_EXECUTION_PROTECTABLE or
> EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE set.
... Are you saying that, if we modify AddMemoryBaseSizeHob() in
OvmfPkg/PlatformPei, to report
EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE (*), then
gDS->SetMemorySpaceAttributes() will start to work?

((*) "MdePkg/Include/Pi/PiHob.h": "NOTE: Since PI spec 1.4, please use
EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE as Memory capability
attribute: The memory supports being protected from processor writes")

If that's the case, then I hope Jian can include such an OvmfPkg patch
in his series. :)

Thanks!
Laszlo

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com] 
> Sent: Thursday, September 14, 2017 4:54 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Wolman, Ayellet <ayellet.wolman@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled.
> 
> On 09/14/17 10:46, Wang, Jian J wrote:
>> It’s an implementation limitation. All page attributes will be filtered out before calling CPU arch protocol to update the attributes (Gcd.c).
> 
> That cannot be a random occurrence; I'm sure there was some specific intent behind filtering out the page attributes.
> 
> Does "git blame" provide a reason, from the message of the commit that added the filtering?
> 
> Thanks,
> Laszlo
> 
>> From: Yao, Jiewen
>> Sent: Thursday, September 14, 2017 4:38 PM
>> To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J 
>> <jian.j.wang@intel.com>
>> Cc: Dong, Eric <eric.dong@intel.com>; Justen, Jordan L 
>> <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Wolman, Ayellet 
>> <ayellet.wolman@intel.com>; Kinney, Michael D 
>> <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>
>> Subject: RE: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled.
>>
>> HI
>> I wonder if it is spec limitation or implementation limitation.
>>
>> If it is implementation limitation, we can enhance the DxeCore to allow it.
>>
>> Thank you
>> Yao Jiewen
>>
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Thursday, September 14, 2017 4:30 PM
>> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
>> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; 
>> Justen, Jordan L 
>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; 
>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen 
>> <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wolman, Ayellet 
>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, 
>> Michael D 
>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Zeng, 
>> Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>
>> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled.
>>
>> On 09/14/17 05:17, Wang, Jian J wrote:
>>> For the use of arch protocol, there's one thing I mentioned is not 
>>> accurate. I actually tried gDS->SetMemorySpaceAttributes() but it 
>>> cannot change page attributes. That's why I have to turn to cpu arch 
>>> protocol.
>> Thank you for the explanation.
>>
>> In that case, we cannot avoid violating the PI spec. I agree we can 
>> break the spec if it happens for security purposes, but in that case, 
>> I believe that hiding the implementation behind a library class is 
>> mandatory. Viewed from the spec side, accessing the CPU Arch protocol 
>> is a hack, and so it should not be spread to the source code of 
>> several driver modules.
>>
>> Thank you,
>> Laszlo
>>
>>
>>>
>>> -----Original Message-----
>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf 
>>> Of Wang, Jian J
>>> Sent: Thursday, September 14, 2017 9:17 AM
>>> To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
>>> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; 
>>> Justen, Jordan L 
>>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; 
>>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen 
>>> <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wolman, Ayellet 
>>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Kinney, 
>>> Michael D 
>>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; 
>>> Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>
>>> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled.
>>>
>>> Thanks for the comments and good advices. Sorry the format issues.
>>> This is my first patch for this project. Too many details for me to 
>>> get familiar with.
>>>
>>> (1) Sure.
>>> (2) I'll change that.
>>> (3) I'll use the tool to ensure the patch format.
>>> (4) I'll remove the ',' in name
>>> (5) I'll add more description about it.
>>> (6) You're right. I should use SetMemorySpaceAttributes() of DXE service
>>>      instead. The only reason I didn't do it is that I found
>>>       GetMemorySpaceDescriptor() doesn't return the same information
>>>      which SetMemorySpaceAttributes() just changed. So I feel using CPU
>>>     arch protocol is a bit safer. Anyway, I'll change it.
>>> (7) I did put those macros in the install function before. To reduce the
>>>      number of changed files, I made current changes. You're right it's
>>>      not worthy.
>>> (8) Using macro can help the readability, which is more important to me.
>>>     I know function can do the same. But it looks a bit heavy in this situation.
>>>     I have to admit replacing  the macros with a library is a very good idea,
>>>     which brings the same readability. I didn't think of that before. Although
>>>     Library is still a little bit heavy to me but it's in a different way, I think it
>>>     worth a trying.
>>> (9) Putting a space before open parenthesis is forced style? If so, I'll add it.
>>> (10) You're right. Using library can reduce the disturbs to affected drivers
>>>        by this feature to the minimum.
>>>
>>> -----Original Message-----
>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>> Sent: Thursday, September 14, 2017 7:35 AM
>>> To: Wang, Jian J 
>>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
>>> Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Justen, 
>>> Jordan L 
>>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; Dong, 
>>> Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Kinney, 
>>> Michael D 
>>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; 
>>> Wolman, Ayellet 
>>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>; Yao, 
>>> Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Zeng, 
>>> Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>
>>> Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled.
>>>
>>> Hi,
>>>
>>> some of the points I'm going to make have already been pointed out by
>>> Jordan:
>>>
>>> (1) When posting a patch series, please collect the Cc: tags from all 
>>> of the patches, and add them *all* to the cover letter. This way 
>>> everyone will get a personal copy of the general description.
>>>
>>>
>>> (2) The subject line is too long. One possible simplification:
>>>
>>> OvmfPkg/QemuVideoDxe: bypass NULL pointer detection
>>>
>>>
>>> On 09/13/17 11:25, Wang, Jian J wrote:
>>>> QemuVideoDxe driver will install VBE SHIM into page 0. If NULL 
>>>> pointer detection is enabled, page 0 must be enabled temporarily 
>>>> before installing and disabled again afterwards. For Windows 7 boot, 
>>>> BIT7 of PcdNullPointerDetectionPropertyMask must still be set to avoid hang.
>>>
>>> (3) Subject line and commit message both should not exceed 74 
>>> characters line length. (Not sure how many chars PatchCheck.py 
>>> actually enforces, I always stick with 74, following the Linux kernel 
>>> tradition.)
>>>
>>> I rewrapped the commit message here for readability.
>>>
>>>
>>>>
>>>> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
>>>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
>>>> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>>
>>>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
>>>> Cc: Justen, Jordan L 
>>>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>
>>>> Cc: Kinney, Michael D 
>>>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
>>>> Cc: Wolman, Ayellet 
>>>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>
>>>> Suggested-by: Wolman, Ayellet 
>>>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Signed-off-by: Wang, Jian J 
>>>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
>>>
>>> (4) I think this is also something that Jordan had pointed out a long 
>>> time ago (apologies if I mis-remember):
>>>
>>> The strings after the tags should form correct email addresses, and 
>>> if there are various email meta-characters in them, like "." (dot) and ","
>>> (comma), then they should be quoted, like this:
>>>
>>> Cc: "Justen, Jordan L" 
>>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>
>>> Cc: "Kinney, Michael D" 
>>> <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
>>> Cc: "Wolman, Ayellet" 
>>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>
>>> Suggested-by: "Wolman, Ayellet" 
>>> <ayellet.wolman@intel.com<mailto:ayellet.wolman@intel.com>>
>>> Signed-off-by: "Wang, Jian J" 
>>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
>>>
>>> If you look at the actual addresses on the emails that have been sent 
>>> out, you can see they are all malformed. For example, I have:
>>>
>>> "Jordan L 
>>> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>" for Jordan -- the part before the comma was taken to be a separate email address (a malformed one).
>>>
>>> At least for my reply, I have fixed up the email addresses.
>>>
>>>
>>> (5) The commit message mentions BIT7 of the new PCD.
>>>
>>> First, thanks for checking Windows 7 boot (and I'm happy that I got 
>>> suspicious of the feature with regard to Windows 7). I think BIT7 is 
>>> a good feature.
>>>
>>> However, please include the short description of that feature in the 
>>> commit message -- it is one sentence; "Disable NULL pointer detection 
>>> just after EndOfDxe."
>>>
>>> (I think BIT7 is a really smart feature, and I like *how* it is used 
>>> in "NULL_DETECTION_ENABLED" below. The check means, "if the 
>>> protection is enabled for DXE, and *not disabled* (== also enabled) after End-of-Dxe".
>>>
>>> This doesn't mean that I like the NULL_DETECTION_ENABLED macro 
>>> itself; more on that below.)
>>>
>>>
>>>> ---
>>>>  OvmfPkg/QemuVideoDxe/Driver.c         | 15 ++++++++++++++-
>>>>  OvmfPkg/QemuVideoDxe/Qemu.h           | 16 ++++++++++++++++
>>>>  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf |  2 ++
>>>>  3 files changed, 32 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c 
>>>> b/OvmfPkg/QemuVideoDxe/Driver.c index 0dce80e59b..ee0eed7214 100644
>>>> --- a/OvmfPkg/QemuVideoDxe/Driver.c
>>>> +++ b/OvmfPkg/QemuVideoDxe/Driver.c
>>>> @@ -194,6 +194,7 @@ QemuVideoControllerDriverStart (
>>>>    PCI_TYPE00                        Pci;
>>>>    QEMU_VIDEO_CARD                   *Card;
>>>>    EFI_PCI_IO_PROTOCOL               *ChildPciIo;
>>>> +  EFI_CPU_ARCH_PROTOCOL             *Cpu;
>>>
>>> (6) I believe I mentioned this in the earlier discussion, in some 
>>> form, but now I'll say it again:
>>>
>>> A UEFI driver has no business poking at the CPU Arch protocol. The PI 
>>> spec (1.6) states,
>>>
>>>   12.3 CPU Architectural Protocol
>>>   EFI_CPU_ARCH_PROTOCOL
>>>
>>>   Summary
>>>
>>>   Abstracts the processor services that are required to implement some
>>>   of the DXE services. This protocol must be produced by a boot service
>>>   or runtime DXE driver and may only be consumed by the DXE Foundation
>>>   and DXE drivers that produce architectural protocols.
>>>
>>> The DXE core is obviously free to use the CPU Arch protocol, but a 
>>> UEFI driver is forbidden from it, *even if* we say that, in this UEFI 
>>> driver, we are going to use DXE services. (Which come from the PI 
>>> spec, and not the UEFI spec.)
>>>
>>> Therefore, here we have to use gDS->SetMemorySpaceAttributes().
>>>
>>> The gDS->SetMemorySpaceAttributes() service depends on the CPU Arch 
>>> protocol, by the PI spec. It is not easy to see, because the PI spec 
>>> has a formatting error in this area. If you look under 
>>> GetMemorySpaceDescriptor(), there is an error code
>>>
>>>   EFI_NOT_AVAILABLE_YET  The attributes cannot be set because CPU
>>>                          architectural protocol is not available yet.
>>>
>>> Obviously this error code belongs to SetMemorySpaceAttributes(), not 
>>> GetMemorySpaceDescriptor().
>>>
>>> Anyway, gDS should be used, architectural protocols shouldn't be.
>>>
>>>
>>>>
>>>>    OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
>>>>
>>>> @@ -479,7 +480,19 @@ QemuVideoControllerDriverStart (  #if defined 
>>>> MDE_CPU_IA32 || defined MDE_CPU_X64
>>>>    if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO ||
>>>>        Private->Variant == QEMU_VIDEO_BOCHS) {
>>>> -    InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase);
>>>> +    //
>>>> +    // Prepare CPU arch protocol for NULL pointer detection
>>>> +    //
>>>> +    Status = gBS->LocateProtocol (
>>>> +                    &gEfiCpuArchProtocolGuid,
>>>> +                    NULL,
>>>> +                    (VOID **) &Cpu
>>>> +                    );
>>>> +    ASSERT_EFI_ERROR (Status);
>>>> +
>>>> +    DISABLE_NULL_DETECTION(Cpu);
>>>> +      InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBufferBase);
>>>> +    ENABLE_NULL_DETECTION(Cpu);
>>>
>>> (7) The NULL detection disabling and enabling should bracket the 
>>> affected code as tightly as possible.
>>>
>>> So please move this into InstallVbeShim() accordingly.
>>>
>>>
>>>>    }
>>>>  #endif
>>>>
>>>> diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h 
>>>> b/OvmfPkg/QemuVideoDxe/Qemu.h index 7fbb25b3ef..bb3bc6eb0f 100644
>>>> --- a/OvmfPkg/QemuVideoDxe/Qemu.h
>>>> +++ b/OvmfPkg/QemuVideoDxe/Qemu.h
>>>> @@ -25,6 +25,7 @@
>>>>  #include <Protocol/PciIo.h>
>>>>  #include <Protocol/DriverSupportedEfiVersion.h>
>>>>  #include <Protocol/DevicePath.h>
>>>> +#include <Protocol/Cpu.h>
>>>>
>>>>  #include <Library/DebugLib.h>
>>>>  #include <Library/UefiDriverEntryPoint.h> @@ -82,6 +83,21 @@ 
>>>> typedef struct {
>>>>
>>>>  #define GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER  0xffff
>>>>
>>>> +//
>>>> +// VBE code will access memory between 0-4095 which will cause page 
>>>> +fault exception // if NULL pointer detection mechanism is enabled. 
>>>> +Following macros can be used to // disable/enable NULL pointer detection before/after accessing those memory.
>>>> +//
>>>> +#define NULL_DETECTION_ENABLED  ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0)
>>>> +#define DISABLE_NULL_DETECTION(Cpu)                                             \
>>>> +  if (NULL_DETECTION_ENABLED) {                                                 \
>>>> +    (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, 0);                     \
>>>> +  }
>>>> +#define ENABLE_NULL_DETECTION(Cpu)                                              \
>>>> +  if (NULL_DETECTION_ENABLED) {                                                 \
>>>> +    (Cpu)->SetMemoryAttributes((Cpu), 0, EFI_PAGE_SIZE, EFI_MEMORY_RP);         \
>>>> +  }
>>>> +
>>>
>>> (8) I believe Jordan too commented on these macros elsewhere (under 
>>> patch 1/4).
>>>
>>> In my opinion, this functionality should be extracted into a library 
>>> class, with a library instance that is suitable for at least 
>>> UEFI_DRIVER modules. (Maybe even for DXE_DRIVER modules.)
>>>
>>> You could add a separate library instance for SMM drivers, if that 
>>> were necessary.
>>>
>>>
>>> (9) Style comment: please put one space character between the 
>>> function designator and the opening parenthesis.
>>>
>>>
>>>>  //
>>>>  // QEMU Video Private Data Structure  // diff --git 
>>>> a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf 
>>>> b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
>>>> index 7c7d429bca..5d166eb99c 100644
>>>> --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
>>>> +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
>>>> @@ -72,7 +72,9 @@
>>>>    gEfiGraphicsOutputProtocolGuid                # PROTOCOL BY_START
>>>>    gEfiDevicePathProtocolGuid                    # PROTOCOL BY_START
>>>>    gEfiPciIoProtocolGuid                         # PROTOCOL TO_START
>>>> +  gEfiCpuArchProtocolGuid
>>>>
>>>>  [Pcd]
>>>>    gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion
>>>> +  
>>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
>>>
>>> (10) Instead of these, the library class that I described under (8) 
>>> should be added here.
>>>
>>> Any further dependencies like PCDs, protocols etc should be inherited 
>>> by the driver through the library instance that the platform DSC file 
>>> resolves the library class to.
>>>
>>> Bonus: should you realize that the feature is impossible to implement 
>>> without accessing the CPU Arch protocol directly, you could hide the 
>>> protocol GUID dependency in the library instance INF file, and I'd be 
>>> none the wiser.
>>>
>>> ... Well, I could at least pretend that. :)
>>>
>>> Thanks,
>>> Laszlo
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>
>