From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 682B821E43B64 for ; Fri, 22 Sep 2017 04:47:19 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C2531285B9; Fri, 22 Sep 2017 11:50:26 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C2531285B9 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-219.rdu2.redhat.com [10.10.120.219]) by smtp.corp.redhat.com (Postfix) with ESMTP id BB2DD5C684; Fri, 22 Sep 2017 11:50:24 +0000 (UTC) To: Jian J Wang , edk2-devel@lists.01.org Cc: Star Zeng , Eric Dong , Jiewen Yao , Michael Kinney , Jordan Justen , Ayellet Wolman References: <20170921052032.13652-1-jian.j.wang@intel.com> <20170921052032.13652-7-jian.j.wang@intel.com> From: Laszlo Ersek Message-ID: <6ed8b38e-3387-e3a1-972f-5922470ed4c7@redhat.com> Date: Fri, 22 Sep 2017 13:50:23 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20170921052032.13652-7-jian.j.wang@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Fri, 22 Sep 2017 11:50:26 +0000 (UTC) Subject: Re: [PATCH v2 6/6] OvmfPkg/QemuVideoDxe: Bypass NULL pointer detection during VBE SHIM installing X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 22 Sep 2017 11:47:19 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Eric Dong > Cc: Laszlo Ersek > Cc: Jiewen Yao > Cc: Michael Kinney > Cc: Jordan Justen > Cc: Ayellet Wolman > Suggested-by: Ayellet Wolman > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang > --- > 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 (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 > #include > #include > #include > +#include > + > #include > > #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