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

[Laszlo Ersek <lersek@redhat.com>]

On 09/22/17 13:50, Laszlo Ersek wrote:
> 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.

Actually, let me take a step back, and re-think the necessity of all
this work for QemuVideoDxe!

The facts are:

(1) The *only* purpose of the VBE Shim is to allow Windows 7 to boot in
pure UEFI mode (i.e. without a CSM).

(2) If I understand correctly, you guys have verified that Windows 7
cannot boot with the page0 protection enabled, *regardless* of what we
do in QemuVideoDxe. Can you confirm this please?

With the above in mind, let's consider the effects of the
"PcdNullPointerDetectionPropertyMask" bits:

* BIT0 clear:
  - The page0 protection is completely disabled.
  - This patch does nothing, in effect.
  - The VBE Shim works.
  - Windows 7 boots.

* BIT0 set, BIT7 also set:
  - The page0 protection is disabled in the DXE core at the end of DXE.
  - This patch does nothing, in effect.
  - The VBE Shim works, because it is a UEFI driver, and it connects its
    devices (and installs the shim) after End-of-Dxe, at which point
    page0 protection is no longer in effect.
  - Windows 7 boots fine, again because it is loaded after End-of-Dxe.

* BIT0 set, BIT7 clear:
  - The page0 protection is never disabled until the OS (loader)
    installs its own page tables.
  - This patch enables the VBE Shim to work, by temporarily disabling
    page0 protection.
  - However, Windows 7 will fail to boot nonetheless, because it cannot
    cope with page0 protection. (This is fact (2).)

Now, if you consider fact (1) as well: given that Windows 7 cannot boot
with page0 protection enabled *anyway*, why mess with the VBE Shim at
all?

How about the following patch instead:

> diff --git a/OvmfPkg/QemuVideoDxe/VbeShim.c b/OvmfPkg/QemuVideoDxe/VbeShim.c
> index e45a08e8873f..8ba5522cde3c 100644
> --- a/OvmfPkg/QemuVideoDxe/VbeShim.c
> +++ b/OvmfPkg/QemuVideoDxe/VbeShim.c
> @@ -75,6 +75,20 @@ InstallVbeShim (
>    UINTN                Printed;
>    VBE_MODE_INFO        *VbeModeInfo;
>
> +  if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) {
> +    DEBUG ((
> +      DEBUG_WARN,
> +      "%a: page 0 protected, not installing VBE shim\n",
> +      __FUNCTION__
> +      ));
> +    DEBUG ((
> +      DEBUG_WARN,
> +      "%a: page 0 protection prevents Windows 7 from booting anyway\n",
> +      __FUNCTION__
> +      ));
> +    return;
> +  }
> +
>    Segment0 = 0x00000;
>    SegmentC = 0xC0000;
>    SegmentF = 0xF0000;

Thanks!
Laszlo