QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer
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.

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>
 #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);
+  }
+
   //
   // 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__));
 }
-- 
2.14.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

You're right that there's no such need. I just saw that this driver is loaded
before EndOfDxe but missed the fact that it's actually started after that.
So BIT7 of PcdNullPointerDetectionPropertyMask is enough.

And thanks a lot for other feedbacks in another emails, especially for the 
catching of potential attributes overridden issue, which also exists in other 
part of code. 

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, September 22, 2017 11:29 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Wolman, Ayellet <ayellet.wolman@intel.com>
> Subject: Re: [PATCH v2 6/6] OvmfPkg/QemuVideoDxe: Bypass NULL pointer
> detection during VBE SHIM installing
> 
> 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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel