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 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
>>