On 09/14/17 11:39, Zeng, Star wrote: > There is a realistic problem I just found and want to share. > > GCD database is built based on resource hob reported from PEI, and > GCD checks the input Attributes against Capabilities when setting > Attributes, but we (I just searched the whole codebase) can see there > is no code reporting resource hob with > EFI_RESOURCE_ATTRIBUTE_READ_PROTECTABLE, > EFI_RESOURCE_ATTRIBUTE_EXECUTION_PROTECTABLE or > EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE set. ... Are you saying that, if we modify AddMemoryBaseSizeHob() in OvmfPkg/PlatformPei, to report EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE (*), then gDS->SetMemorySpaceAttributes() will start to work?
((*) "MdePkg/Include/Pi/PiHob.h": "NOTE: Since PI spec 1.4, please use EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE as Memory capability attribute: The memory supports being protected from processor writes") If that's the case, then I hope Jian can include such an OvmfPkg patch in his series. :) Thanks! Laszlo > -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: Thursday, September 14, 2017 4:54 PM > To: Wang, Jian J <[email protected]>; Yao, Jiewen <[email protected]> > Cc: Dong, Eric <[email protected]>; Justen, Jordan L > <[email protected]>; [email protected]; Wolman, Ayellet > <[email protected]>; Kinney, Michael D <[email protected]>; > Zeng, Star <[email protected]> > Subject: Re: [edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe > driver to bypass NULL pointer detection if enabled. > > 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 <[email protected]>; Wang, Jian J >> <[email protected]> >> Cc: Dong, Eric <[email protected]>; Justen, Jordan L >> <[email protected]>; [email protected]; Wolman, Ayellet >> <[email protected]>; Kinney, Michael D >> <[email protected]>; Zeng, Star <[email protected]> >> 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:[email protected]] >> Sent: Thursday, September 14, 2017 4:30 PM >> To: Wang, Jian J <[email protected]<mailto:[email protected]>> >> Cc: Dong, Eric <[email protected]<mailto:[email protected]>>; >> Justen, Jordan L >> <[email protected]<mailto:[email protected]>>; >> [email protected]<mailto:[email protected]>; Yao, Jiewen >> <[email protected]<mailto:[email protected]>>; Wolman, Ayellet >> <[email protected]<mailto:[email protected]>>; Kinney, >> Michael D >> <[email protected]<mailto:[email protected]>>; Zeng, >> Star <[email protected]<mailto:[email protected]>> >> 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:[email protected]] On Behalf >>> Of Wang, Jian J >>> Sent: Thursday, September 14, 2017 9:17 AM >>> To: Laszlo Ersek <[email protected]<mailto:[email protected]>> >>> Cc: Dong, Eric <[email protected]<mailto:[email protected]>>; >>> Justen, Jordan L >>> <[email protected]<mailto:[email protected]>>; >>> [email protected]<mailto:[email protected]>; Yao, Jiewen >>> <[email protected]<mailto:[email protected]>>; Wolman, Ayellet >>> <[email protected]<mailto:[email protected]>>; Kinney, >>> Michael D >>> <[email protected]<mailto:[email protected]>>; >>> Zeng, Star <[email protected]<mailto:[email protected]>> >>> 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:[email protected]] >>> Sent: Thursday, September 14, 2017 7:35 AM >>> To: Wang, Jian J >>> <[email protected]<mailto:[email protected]>> >>> Cc: [email protected]<mailto:[email protected]>; Justen, >>> Jordan L >>> <[email protected]<mailto:[email protected]>>; Dong, >>> Eric <[email protected]<mailto:[email protected]>>; Kinney, >>> Michael D >>> <[email protected]<mailto:[email protected]>>; >>> Wolman, Ayellet >>> <[email protected]<mailto:[email protected]>>; Yao, >>> Jiewen <[email protected]<mailto:[email protected]>>; Zeng, >>> Star <[email protected]<mailto:[email protected]>> >>> 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 <[email protected]<mailto:[email protected]>> >>>> Cc: Eric Dong <[email protected]<mailto:[email protected]>> >>>> Cc: Star Zeng <[email protected]<mailto:[email protected]>> >>>> Cc: Laszlo Ersek <[email protected]<mailto:[email protected]>> >>>> Cc: Justen, Jordan L >>>> <[email protected]<mailto:[email protected]>> >>>> Cc: Kinney, Michael D >>>> <[email protected]<mailto:[email protected]>> >>>> Cc: Wolman, Ayellet >>>> <[email protected]<mailto:[email protected]>> >>>> Suggested-by: Wolman, Ayellet >>>> <[email protected]<mailto:[email protected]>> >>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>> Signed-off-by: Wang, Jian J >>>> <[email protected]<mailto:[email protected]>> >>> >>> (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" >>> <[email protected]<mailto:[email protected]>> >>> Cc: "Kinney, Michael D" >>> <[email protected]<mailto:[email protected]>> >>> Cc: "Wolman, Ayellet" >>> <[email protected]<mailto:[email protected]>> >>> Suggested-by: "Wolman, Ayellet" >>> <[email protected]<mailto:[email protected]>> >>> Signed-off-by: "Wang, Jian J" >>> <[email protected]<mailto:[email protected]>> >>> >>> 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 >>> <[email protected]<mailto:[email protected]>>" 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 >>> [email protected]<mailto:[email protected]> >>> https://lists.01.org/mailman/listinfo/edk2-devel >>> > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
