Re: [PATCH v2 6/6] OvmfPkg/QemuVideoDxe: Bypass NULL pointer detection during VBE SHIM installing

[Laszlo Ersek <lersek@redhat.com>]

This patch looks great to me, I would like to request a few small updates:

On 09/21/17 07:20, Jian J Wang wrote:
> QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer

(1) please replace the word "install" with "link".

The VBE Shim is technically installed into the "real-mode" C segment,
only the int 0x10 vector lives in page 0.

> detection is enabled, this driver will fail to load. NULL pointer detection
> bypassing code is added to prevent such problem during boot.
> 
> Please note that Windows 7 will try to access VBE SHIM during boot if it's
> installed, and then cause boot failure. This can be fixed by setting BIT7
> of PcdNullPointerDetectionPropertyMask to disable NULL pointer detection
> after EndOfDxe. As far as we know, there's no other OSs has such issue.

This is not a request, just a comment: I verified the default value in
the .dec, and I see it is 0. So there's no need to post an additional
patch for the OVMF DSC files, in order to set BIT7.

> 
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ayellet Wolman <ayellet.wolman@intel.com>
> Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf |  2 ++
>  OvmfPkg/QemuVideoDxe/VbeShim.c        | 22 ++++++++++++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
> index 577e07b0a8..8078232ded 100644
> --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
> +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
> @@ -67,6 +67,7 @@
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
>    UefiLib
> +  DxeServicesTableLib
>  
>  [Protocols]
>    gEfiDriverSupportedEfiVersionProtocolGuid     # PROTOCOL ALWAYS_PRODUCED
> @@ -77,3 +78,4 @@
>  [Pcd]
>    gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
> diff --git a/OvmfPkg/QemuVideoDxe/VbeShim.c b/OvmfPkg/QemuVideoDxe/VbeShim.c
> index e45a08e887..c3fb6d8d3c 100644
> --- a/OvmfPkg/QemuVideoDxe/VbeShim.c
> +++ b/OvmfPkg/QemuVideoDxe/VbeShim.c
> @@ -21,10 +21,13 @@
>    WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  **/
>  
> +#include <Pi/PiDxeCis.h>

(2) Question: what exactly is this necessary for?

(I would think that "DxeServicesTableLib.h" gave you everything you
needed, but I could be wrong.)

>  #include <IndustryStandard/LegacyVgaBios.h>
>  #include <Library/DebugLib.h>
>  #include <Library/PciLib.h>
>  #include <Library/PrintLib.h>
> +#include <Library/DxeServicesTableLib.h>
> +
>  #include <OvmfPlatforms.h>
>  
>  #include "Qemu.h"
> @@ -74,11 +77,21 @@ InstallVbeShim (
>    UINT8                *Ptr;
>    UINTN                Printed;
>    VBE_MODE_INFO        *VbeModeInfo;
> +  EFI_STATUS           Status;
>  
>    Segment0 = 0x00000;
>    SegmentC = 0xC0000;
>    SegmentF = 0xF0000;
>  
> +  //
> +  // Disable NULL pointer detection temporarily. Otherwise the installation
> +  // will fail due to the lack of memory access right.
> +  //
> +  if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) {
> +    Status = gDS->SetMemorySpaceAttributes (0, EFI_PAGES_TO_SIZE (1), 0);
> +    ASSERT_EFI_ERROR (Status);
> +  }
> +

(3) Please hoist the

  Segment0Pages = 1;

assignment from below, and use it in the SetMemorySpaceAttributes()
call, for the "Length" argument.

(4) Please use the variable "Segment0" as the "BaseAddress" argument.

(5) The Attributes=0 argument surprises me, and the end of this patch
seems to confirm that I'm right to be surprised :)

Instead of setting 0, can you please
- first get the original attributes with GetMemorySpaceDescriptor(),
- then clear only the attributes here that prevent read/write access,
- and at the end of the function, restore the original attributes?

I think this can be done without dynamic memory allocation, you just
need a local EFI_GCD_MEMORY_SPACE_DESCRIPTOR object.

>    //
>    // Attempt to cover the real mode IVT with an allocation. This is a UEFI
>    // driver, hence the arch protocols have been installed previously. Among
> @@ -304,5 +317,14 @@ InstallVbeShim (
>    Int0x10->Segment = (UINT16) ((UINT32)SegmentC >> 4);
>    Int0x10->Offset  = (UINT16) ((UINTN) (VbeModeInfo + 1) - SegmentC);
>  
> +  //
> +  // Get NULL pointer detection back
> +  //
> +  if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) {
> +    Status = gDS->SetMemorySpaceAttributes (0, EFI_PAGES_TO_SIZE(1),
> +                                            EFI_MEMORY_RP);
> +    ASSERT_EFI_ERROR (Status);
> +  }
> +
>    DEBUG ((EFI_D_INFO, "%a: VBE shim installed\n", __FUNCTION__));
>  }
> 

(6) The InstallVbeShim() function actually contains *two* earlier exits
than this. Please search the function for "return" statements.

I suggest the following:

- put the restoration of the original page attributes at the very end of
the function,
- put a label called "RestoreSegment0Attributes" between the DEBUG
message and the page attributes restoration,
- replace the "return" statements with "goto RestoreSegment0Attributes".

Thanks!
Laszlo