Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling MTRRs prior to MTRR change.
On 9/18/2018 10:57 PM, Duran, Leo wrote: -Original Message- From: Ni, Ruiyu [mailto:ruiyu...@intel.com] Sent: Tuesday, September 18, 2018 3:34 AM To: Laszlo Ersek ; Duran, Leo ; edk2-devel@lists.01.org Cc: Dong, Eric Subject: Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling MTRRs prior to MTRR change. On 9/18/2018 12:38 AM, Laszlo Ersek wrote: On 09/17/18 18:20, Duran, Leo wrote: -Original Message- From: Ni, Ruiyu Sent: Thursday, September 13, 2018 11:44 PM To: Duran, Leo ; Laszlo Ersek ; edk2-devel@lists.01.org Cc: Dong, Eric Subject: Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling MTRRs prior to MTRR change. On 9/14/2018 3:31 AM, Duran, Leo wrote: -Original Message- From: Ni, Ruiyu Sent: Wednesday, September 12, 2018 9:39 PM To: Duran, Leo ; Laszlo Ersek ; edk2-devel@lists.01.org Cc: Dong, Eric Subject: RE: [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling MTRRs prior to MTRR change. Leo, Sorry I was in leave yesterday so didn't see the mail. Which MSRs are shared? Only the MSR_IA32_MTRR_DEF_TYPE_REGISTER? Or all the MSRs that configures the CPU MTRR setting? Hi Ray, The MTTR config MSRs are also shared by threads within a core. Hi Leo, Do you think that fixing the caller is more proper? Hi Ray, Actually, The proposed PCD is the simplest solution, as that works for us and does not change the existing (default) flow. That is, I'd prefer making a decision about the PCD in platform-specific code, rather than introducing complex detection and heuristics at the caller level in EDK2 (just for AMD). So, please approve the PCD. Leo, I agree with you on the first part "the PCD is the simplest solution". But this really looks like a workaround of the real issue. For a multiple-socket system, it may contain S sockets, each socket contains C cores and each core contains T threads. In summary the system contains S * C * T threads. As you said all threads inside a core share the MTRR setting. Do all cores inside a socket share the MTRR setting? Do all sockets share the MTRR setting? If one of the answer of above questions is "no", how can we configure the PCD? [Duran, Leo] Hi Ray, The MTTR settings are share by threads within a core (but each core has its own, etc.) The PCD would be set in our platform-specific code (e.g., it can be set at build-time in the .DSC file). As I mentioned, We don't need (Mtrr.Enable=0) to change MTRR settings, so having the PCD to skip (Mtrr.Enable=0) is reasonable for us. Leo. If the PCD is false, no thread disables the MTRR before programming it. Is it safe? Per Intel's SDM, it's not. Maybe it works in AMD's case. But I still suggest we change the caller, which is more natural. At least I'd like to see how potential-ugly the change can be. We can then discuss how to make the ugly change better looking. - From my side, if it works for you, it works for me. (The general trend has been to avoid adding more PCDs to the "core" package DEC files, but I'm 100% neutral on that.) Laszlo Laszlo, Thanks for pointing out the general trend. Yes less PCDs are very welcomed. To me, PCD is no different from protocol. And even worse, because it's very easily to be over-used. But I am not sure whether a PCD has to be introduced for this issue. Maybe even we choose to fix the caller, the PCD is still needed. I am not sure. -- Thanks, Ray ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel -- Thanks, Ray ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdeModulePkg/DxeIpl: support more NX related PCDs
If no more new comments, I'll do following changes in v2, including review comments got so far: a. change ToEnableExecuteDisableFeature() to EnableNonExec() b. remove the ASSERT and DEBUG in current ToEnableExecuteDisableFeature() c. update dec/uni file to clarify the usage of following PCDs PcdNxSetForStack TRUE - Apply NX to stack memory FALSE - Don't care of protection of stack memory PcdImageProtectionPolicy 1 - Apply NX to data section of image from the corresponding origin 0 - Don't care of the protection of data section of image from the corresponding origin PcdDxeNxMemoryProtectionPolicy 1 - Apply NX to corresponding type of memory 0 - Don't care of the protection of corresponding type of memory Regards, Jian > -Original Message- > From: Zeng, Star > Sent: Tuesday, September 18, 2018 4:47 PM > To: Wang, Jian J ; Laszlo Ersek ; > edk2-devel@lists.01.org > Cc: Ard Biesheuvel ; Ni, Ruiyu > ; > Yao, Jiewen ; Zeng, Star > Subject: RE: [PATCH] MdeModulePkg/DxeIpl: support more NX related PCDs > > I totally agree adding more clear documentation in dec and uni. > My only concern is that the warning message (by checking the combinations) to > explain in c may bring more confusion. > Anyway, if we can have the warning message to make the things more clear, I > definitely agree it. :) > > > Thanks, > Star > -Original Message- > From: Wang, Jian J > Sent: Tuesday, September 18, 2018 9:22 AM > To: Laszlo Ersek ; Zeng, Star ; edk2- > de...@lists.01.org > Cc: Ard Biesheuvel ; Ni, Ruiyu > ; > Yao, Jiewen > Subject: RE: [PATCH] MdeModulePkg/DxeIpl: support more NX related PCDs > > I have no strong opinion for this proposal. But if we decide to do it > finally, I'd > suggest to add some warning messages for any probably surprising setting > combinations. > > Regards, > Jian > > > > -Original Message- > > From: Laszlo Ersek [mailto:ler...@redhat.com] > > Sent: Monday, September 17, 2018 6:14 PM > > To: Zeng, Star ; Wang, Jian J > > ; edk2-devel@lists.01.org > > Cc: Ard Biesheuvel ; Ni, Ruiyu > > ; Yao, Jiewen > > Subject: Re: [PATCH] MdeModulePkg/DxeIpl: support more NX related PCDs > > > > On 09/17/18 07:57, Zeng, Star wrote: > > > How about we see the problem in another way? > > > > > > If my understanding is correct, current discussion and patches think > > > FALSE/0 > > means disable/clear NX, but that is not the fact. > > > According to the code implementation, FALSE/0 seems mean *AS IS* to > > > do > > thing (no code to disable/clear NX). > > > > > > PcdSetNxForStack > > > TRUE: Set NX for stack. > > > FALSE: No code to clear NX for stack. > > > > > > PcdDxeNxMemoryProtectionPolicy > > > BITX 1: Set NX for that memory type. > > > BITX 0: No code to clear NX for that memory type. > > > > > > PcdImageProtectionPolicy > > > BITX 1: Set NX for the image data section. > > > BITX 0: Not code to clear NX for the image data section. > > > > > > So, how about we think one PCD just works for itself and it does not > > > impact > > other PCDs to protect? > > > That means TRUE/1 is to protect and FALSE/0 is *AS IS* to do nothing. > > > The description of these PCDs could be enhancement if we think it is > > > a good > > way to see the problem. > > > > Sure, that too could work for me, but then the documentation in the > > DEC / UNI files has to be really clear. > > > > The initial worry for the current discussion was that some platform > > might > > - protect e.g. BootServicesData type memory, > > - not set PcdSetNxForStack, > > - expect the stack to remain executable. > > > > The actual results might surprise the platform owner. > > > > If the documentation dispelled any possible misconceptions, I think > > your idea could work too (and it would be a lot easier to code). > > > > Thanks > > Laszlo > > > > > > > > > > > > > Thanks, > > > Star > > > -Original Message- > > > From: Wang, Jian J > > > Sent: Monday, September 17, 2018 10:11 AM > > > To: Laszlo Ersek ; edk2-devel@lists.01.org > > > Cc: Zeng, Star ; Ard Biesheuvel > > ; Ni, Ruiyu ; Yao, > > Jiewen > > > Subject: RE: [PATCH] MdeModulePkg/DxeIpl: support more NX related > > > PCDs > > > > > > Laszlo, > > > > > > Thanks for the comments. > > > > > > Regards, > > > Jian > > > > > >> -Original Message- > > >> From: Laszlo Ersek [mailto:ler...@redhat.com] > > >> Sent: Friday, September 14, 2018 5:51 PM > > >> To: Wang, Jian J ; edk2-devel@lists.01.org > > >> Cc: Zeng, Star ; Ard Biesheuvel > > >> ; Ni, Ruiyu ; Yao, > > >> Jiewen > > >> Subject: Re: [PATCH] MdeModulePkg/DxeIpl: support more NX related > > >> PCDs > > >> > > >> I've got some comments on the code as well: > > >> > > >> On 09/14/18 07:13, Jian J Wang wrote: > > >>> BZ#1116: https://bugzilla.tianocore.org/show_bug.cgi?id=1116 > > >>> > > >>> Currently IA32_EFER.NXE is only set against PcdSetNxForStack. > > >>> This > > >>> confuses developers because following two other PCDs also ne
Re: [edk2] [PATCH v2 0/7] MdeModulePkg: add support for dispatching foreign arch PE/COFF images
Thank you Ard. Good to know. Did you also try some security test, such as input a bad image to see if the code can return failure gracefully? Or enable secure boot to see if the image verification process still works well ? thank you! Yao, Jiewen > 在 2018年9月18日，下午9:53，Ard Biesheuvel 写道： > >> On 18 September 2018 at 00:32, Yao, Jiewen wrote: >> HI Ard >> This is a great feature. >> > > Thanks! > >> May I know what test has been done for this patch series? >> >> Would you please share that information? No matter your unit test, or system >> level test. >> > > I have used ArmVirtPkg/ArmVirtQemu.dsc built for AARCH64 in two > configurations: > - one replacing the native FAT driver built from source with the EBC > binary version > - another one replacing the FAT driver and including the X86 emulator [0] > > The tests involved accessing a FAT partition (both builds) and running > the X86 version of iPXE (the latter build only) > > > > [0] https://github.com/ardbiesheuvel/X86EmulatorPkg > > >>> -Original Message- >>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of >>> Ard Biesheuvel >>> Sent: Saturday, September 15, 2018 9:29 PM >>> To: edk2-devel@lists.01.org >>> Cc: Ni, Ruiyu ; Zimmer, Vincent >>> ; Dong, Eric ; Andrew >>> Fish ; Carsey, Jaben ; >>> Richardson, Brian ; Gao, Liming >>> ; Kinney, Michael D ; >>> Zeng, Star >>> Subject: [edk2] [PATCH v2 0/7] MdeModulePkg: add support for dispatching >>> foreign arch PE/COFF images >>> >>> Add the basic plumbing to DXE core, the PCI bus driver and the boot manager >>> to allow PE/COFF images to be dispatched that target an architecture that is >>> not native for the platform, but which is supported by one of potentially >>> several available emulators. >>> >>> One implementation of such an emulator can be found here: >>> https://github.com/ardbiesheuvel/X86EmulatorPkg >>> >>> This also allows us to get rid of the special treatment of EBC images in >>> core code. Instead, the EbcDxe driver is augmented with an implementation >>> of the EDK2 PE/COFF image emulator so that internal knowledge of how EBC >>> is implemented (I-cache flushing, thunks) is removed from the DXE core. >>> >>> Changes since v2: >>> - subsume the EBC handling into the EDK2 emulator protocol and abstract >>> away from EBC specifics in core code. >>> - allow multiple emulator implementations to co-exist >>> - incorporate Star's review feedback >>> >>> Cc: Vincent Zimmer >>> Cc: Brian Richardson >>> Cc: Michael D Kinney >>> Cc: Andrew Fish >>> Cc: Leif Lindholm >>> Cc: Star Zeng >>> Cc: Eric Dong >>> Cc: Ruiyu Ni >>> Cc: Liming Gao >>> Cc: Jaben Carsey >>> Cc: Steven Shi >>> >>> Ard Biesheuvel (7): >>> MdeModulePkg: introduce PE/COFF image emulator protocol >>> MdeModulePkg/DxeCore: invoke the emulator protocol for foreign >>> images >>> MdeModulePkg/PciBusDxe: invoke PE/COFF emulator for foreign option >>>ROMs >>> MdeModulePkg/UefiBootManagerLib: allow foreign Driver images >>> MdeModulePkg/EbcDxe: implement the PE/COFF emulator protocol >>> MdePkg/UefiBaseType.h: treat EBC as a non-native machine type >>> MdeModulePkg/DxeCore: remove explicit EBC handling >>> >>> MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h | 1 + >>> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf | 1 + >>> .../Bus/Pci/PciBusDxe/PciOptionRomSupport.c | 51 ++- >>> MdeModulePkg/Core/Dxe/DxeMain.h | 6 +- >>> MdeModulePkg/Core/Dxe/DxeMain.inf | 2 +- >>> MdeModulePkg/Core/Dxe/Image/Image.c | 141 >>> +++--- >>> .../Include/Protocol/PeCoffImageEmulator.h| 99 >>> .../Library/UefiBootManagerLib/BmLoadOption.c | 51 ++- >>> .../Library/UefiBootManagerLib/InternalBm.h | 1 + >>> .../UefiBootManagerLib/UefiBootManagerLib.inf | 1 + >>> MdeModulePkg/MdeModulePkg.dec | 4 + >>> MdeModulePkg/Universal/EbcDxe/EbcDxe.inf | 3 + >>> MdeModulePkg/Universal/EbcDxe/EbcInt.c| 124 >>> +++ >>> MdeModulePkg/Universal/EbcDxe/EbcInt.h| 3 + >>> MdePkg/Include/Uefi/UefiBaseType.h| 8 +- >>> 15 files changed, 432 insertions(+), 64 deletions(-) >>> create mode 100644 >>> MdeModulePkg/Include/Protocol/PeCoffImageEmulator.h >>> >>> -- >>> 2.17.1 >>> >>> ___ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 0/7] MdeModulePkg: add support for dispatching foreign arch PE/COFF images
One more, did you enable tpm to see if tpm measurement still works well ? Also did defer image solution still takes effect with this change? Sorry to ask many questions, I want to make sure the current security design still work with this new capability. thank you! Yao, Jiewen > 在 2018年9月19日，下午5:30，Yao, Jiewen 写道： > > Thank you Ard. Good to know. > > Did you also try some security test, such as input a bad image to see if the > code can return failure gracefully? > > Or enable secure boot to see if the image verification process still works > well ? > > thank you! > Yao, Jiewen > > >>> 在 2018年9月18日，下午9:53，Ard Biesheuvel 写道： >>> >>> On 18 September 2018 at 00:32, Yao, Jiewen wrote: >>> HI Ard >>> This is a great feature. >>> >> >> Thanks! >> >>> May I know what test has been done for this patch series? >>> >>> Would you please share that information? No matter your unit test, or >>> system level test. >>> >> >> I have used ArmVirtPkg/ArmVirtQemu.dsc built for AARCH64 in two >> configurations: >> - one replacing the native FAT driver built from source with the EBC >> binary version >> - another one replacing the FAT driver and including the X86 emulator [0] >> >> The tests involved accessing a FAT partition (both builds) and running >> the X86 version of iPXE (the latter build only) >> >> >> >> [0] https://github.com/ardbiesheuvel/X86EmulatorPkg >> >> -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ard Biesheuvel Sent: Saturday, September 15, 2018 9:29 PM To: edk2-devel@lists.01.org Cc: Ni, Ruiyu ; Zimmer, Vincent ; Dong, Eric ; Andrew Fish ; Carsey, Jaben ; Richardson, Brian ; Gao, Liming ; Kinney, Michael D ; Zeng, Star Subject: [edk2] [PATCH v2 0/7] MdeModulePkg: add support for dispatching foreign arch PE/COFF images Add the basic plumbing to DXE core, the PCI bus driver and the boot manager to allow PE/COFF images to be dispatched that target an architecture that is not native for the platform, but which is supported by one of potentially several available emulators. One implementation of such an emulator can be found here: https://github.com/ardbiesheuvel/X86EmulatorPkg This also allows us to get rid of the special treatment of EBC images in core code. Instead, the EbcDxe driver is augmented with an implementation of the EDK2 PE/COFF image emulator so that internal knowledge of how EBC is implemented (I-cache flushing, thunks) is removed from the DXE core. Changes since v2: - subsume the EBC handling into the EDK2 emulator protocol and abstract away from EBC specifics in core code. - allow multiple emulator implementations to co-exist - incorporate Star's review feedback Cc: Vincent Zimmer Cc: Brian Richardson Cc: Michael D Kinney Cc: Andrew Fish Cc: Leif Lindholm Cc: Star Zeng Cc: Eric Dong Cc: Ruiyu Ni Cc: Liming Gao Cc: Jaben Carsey Cc: Steven Shi Ard Biesheuvel (7): MdeModulePkg: introduce PE/COFF image emulator protocol MdeModulePkg/DxeCore: invoke the emulator protocol for foreign images MdeModulePkg/PciBusDxe: invoke PE/COFF emulator for foreign option ROMs MdeModulePkg/UefiBootManagerLib: allow foreign Driver images MdeModulePkg/EbcDxe: implement the PE/COFF emulator protocol MdePkg/UefiBaseType.h: treat EBC as a non-native machine type MdeModulePkg/DxeCore: remove explicit EBC handling MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h | 1 + MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf | 1 + .../Bus/Pci/PciBusDxe/PciOptionRomSupport.c | 51 ++- MdeModulePkg/Core/Dxe/DxeMain.h | 6 +- MdeModulePkg/Core/Dxe/DxeMain.inf | 2 +- MdeModulePkg/Core/Dxe/Image/Image.c | 141 +++--- .../Include/Protocol/PeCoffImageEmulator.h| 99 .../Library/UefiBootManagerLib/BmLoadOption.c | 51 ++- .../Library/UefiBootManagerLib/InternalBm.h | 1 + .../UefiBootManagerLib/UefiBootManagerLib.inf | 1 + MdeModulePkg/MdeModulePkg.dec | 4 + MdeModulePkg/Universal/EbcDxe/EbcDxe.inf | 3 + MdeModulePkg/Universal/EbcDxe/EbcInt.c| 124 +++ MdeModulePkg/Universal/EbcDxe/EbcInt.h| 3 + MdePkg/Include/Uefi/UefiBaseType.h| 8 +- 15 files changed, 432 insertions(+), 64 deletions(-) create mode 100644 MdeModulePkg/Include/Protocol/PeCoffImageEmulator.h -- 2.17.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel > ___ > edk2-
Re: [edk2] [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get stack pointer
On 09/18/18 20:02, Jordan Justen wrote:
> I guess the git config sendemail.from setting did not help your
> patches. ?? It still is coming through with a From field of
> .
>
> Regarding this patch, I suppose it is worth asking if &StackBase in
> the old code could possibly be an address not on the stack. I don't
> think it is possible, and I'm guessing the C specification would
> probably back that up.
>
> It can be unsafe to get an address of something on the stack and then
> refer to that address after the variable is no longer in scope. I
> suspect this is what the static checker is noticing. By calling
> SetJump, aren't we just doing the same thing, but hiding what we are
> doing from the static checker?
Yep, we're totally doing that.
Thanks,
Laszlo
>
> So, can't we just tell the static checker to ignore the error because
> we know what we are doing?
>
> -Jordan
>
> On 2018-09-18 02:04:48, wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1186
>>
>> This patch uses SetJump() to get the stack pointer from esp/rsp
>> register to replace local variable way, which was marked by static
>> code checker as an unsafe way.
>>
>> Cc: Dandan Bi
>> Cc: Hao A Wu
>> Cc: Eric Dong
>> Cc: Laszlo Ersek
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Jian J Wang
>> ---
>> UefiCpuPkg/CpuMpPei/CpuMpPei.h | 8
>> UefiCpuPkg/CpuMpPei/CpuPaging.c | 9 +++--
>> 2 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.h b/UefiCpuPkg/CpuMpPei/CpuMpPei.h
>> index d097a66aa8..fe61f5e3bc 100644
>> --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.h
>> +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.h
>> @@ -35,6 +35,14 @@
>>
>> extern EFI_PEI_PPI_DESCRIPTOR mPeiCpuMpPpiDesc;
>>
>> +#if defined (MDE_CPU_IA32)
>> +#define CPU_STACK_POINTER(Context) ((Context).Esp)
>> +#elif defined (MDE_CPU_X64)
>> +#define CPU_STACK_POINTER(Context) ((Context).Rsp)
>> +#else
>> +#error CPU type not supported!
>> +#endif
>> +
>> /**
>>This service retrieves the number of logical processor in the platform
>>and the number of those logical processors that are enabled on this boot.
>> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c
>> b/UefiCpuPkg/CpuMpPei/CpuPaging.c
>> index c7e0822452..997c20c26e 100644
>> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
>> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
>> @@ -517,9 +517,14 @@ GetStackBase (
>>IN OUT VOID *Buffer
>>)
>> {
>> - EFI_PHYSICAL_ADDRESSStackBase;
>> + EFI_PHYSICAL_ADDRESS StackBase;
>> + BASE_LIBRARY_JUMP_BUFFER Context;
>>
>> - StackBase = (EFI_PHYSICAL_ADDRESS)(UINTN)&StackBase;
>> + //
>> + // Retrieve stack pointer from current processor context.
>> + //
>> + SetJump (&Context);
>> + StackBase = (EFI_PHYSICAL_ADDRESS)CPU_STACK_POINTER (Context);
>>StackBase += BASE_4KB;
>>StackBase &= ~((EFI_PHYSICAL_ADDRESS)BASE_4KB - 1);
>>StackBase -= PcdGet32(PcdCpuApStackSize);
>> --
>> 2.16.2.windows.1
>>
>> ___
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch v2] NetworkPkg: UefiPxeBcDxe: Add EXCLUSIVE attribute when opening SNP protocol installed by PXE.
On 09/19/18 03:31, Wu, Jiaxin wrote: >>> Subject: [Patch v2] NetworkPkg: UefiPxeBcDxe: Add EXCLUSIVE attribute >> when opening SNP protocol installed by PXE. >>> >>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1152 >>> >>> v2: Sync the same logic to Ipv6 and update code comments. >>> >>> The PXE driver installs a SNP and open this SNP with attribute BY_DRIVER >>> to avoid it being opened by MNP driver, this SNP is also expected not to >>> be opened by other drivers with EXCLUSIVE attribute. In some cases, other >>> drivers may happen to do this by error, and thus cause a system crash. >>> This patch adds EXCLUSIVE attribute when opening SNP in PXE driver, and >>> will reject all OpenProtocol requests by EXCLUSIVE. >>> >>> Cc: Subramanian, Sriram >>> Cc: Ye Ting >>> Cc: Fu Siyuan >>> Cc: Wu Jiaxin >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Wang Fan >>> --- >>> NetworkPkg/UefiPxeBcDxe/PxeBcDriver.c | 8 >>> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> Either my edk2-devel subscription is breaking down, or our discipline >> regarding commit pushing is down the drain, recently. >> >> This patch has been pushed as commit cde5a72d365e. Problems with that >> commit: >> >> (1) The git authorship is marked as "edk2-devel-boun...@lists.01.org". >> That's *wrong*. This patch was authored by Wang Fan >> . >> > > Sorry Laszlo, I didn't check the "Author" field before pushing the patch as I > received the commit request form Fan. I don't know whether it's the patch > reason or TortoiseGit failed to fetch the "Signed-off-by" tag as "Author" > field. Anyway, I will double check that. Thanks. You can always correct the authorship metadata with: git commit --amend --author='Name ' without staging any code changes first. (So that the patch body is not itself modified.) Thanks! Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch 0/5] Support windowsize to benefit tftp/pxe download performance.
On 09/19/18 04:20, Wu, Jiaxin wrote: >> On 09/17/18 07:43, Jiaxin Wu wrote: >>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=886 >>> >>> The series patches are to support the TFTP windowsize option described in >> RFC 7440. >>> TFTP shell command and UEFI PXE driver will use the feature to benefit the >> download >>> performance. >> >> I tested this series, between two virtual machines running on my laptop. >> The TFTP server program that I used was "tftp-server-5.2-22.el7.x86_64". >> The downloaded file was 478,150,656 bytes in size. I built OVMF with >> NETWORK_IP6_ENABLE, so that the last patch would take effect for both >> PXEv4 and PXEv6. >> >> Before the series: >> - PXEv4: 75 seconds (~ 6225 KB/s) >> - PXEv6: 100 seconds (~ 4669 KB/s) >> >> After the series: >> - PXEv4: 48 seconds (~ 9728 KB/s) >> - PXEv6: 60 seconds (~ 7782 KB/s) >> >> These measurements are very rough (I didn't run them multiple times >> etc), but I think they are still quite good indicators. >> >> For the testing, I used the UEFI boot options in UiApp, and not the >> shell command, hence I have no feedback on patch #3. >> >> For patches #1, #2, and #5: >> >> Tested-by: Laszlo Ersek >> > > Thanks the verification. > > >> However, as I pointed out elsewhere in the thread, I think: >> >> - You might want to port the changes from patch#5 to >> "MdeModulePkg/Universal/Network/UefiPxeBcDxe" as well, in a separate >> patch (patch #6). >> >> - If not, then (a) we should document this feature difference in the INF >> files of the UefiPxeBcDxe drivers, (b) patch #4 should be re-done so >> that it target NetworkPkg, not MdeModulePkg. >> > > As I said in the previous email, normally, we only add the new feature into > the combo driver. But I think that's depends on the request. If you want to > include the feature into MdeModulePkg/Universal/Network/UefiPxeBcDxe since > the OVMF platform might use it, I will create patch #6. If not, I will follow > the comments (a)/(b). I don't currently have a use case or "requirement" for the window size feature to work in an OVMF build that does *not* have NETWORK_IP6_ENABLE. So from my side, we can delay the porting until such a need materializes (it might never materialize, of course!) However, because this information -- i.e., the feature separation between the IPv4-only, and the combined IPv4/IPv6 driver -- is new to me, can we please document it somewhere in the code, for example, in the INF files? We don't have to spell out the TFTP window size feature by name, just the fact that NetworkPkg's UefiPxeBcDxe has more features in general (even such features that are orthogonal to internet protocol version, v4 vs. v6). If such documentation already exists, then I missed it, sorry! Thanks! Laszlo >> - Patch #4 (regardless of package DEC) should be extended with >> documentation (both DEC and UNI). >> >> Thanks >> Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdeModulePkg/DxeIpl: support more NX related PCDs
On 09/19/18 11:13, Wang, Jian J wrote: > If no more new comments, I'll do following changes in v2, including review > comments got so far: > > a. change ToEnableExecuteDisableFeature() to EnableNonExec() > b. remove the ASSERT and DEBUG in current ToEnableExecuteDisableFeature() > c. update dec/uni file to clarify the usage of following PCDs > PcdNxSetForStack > TRUE - Apply NX to stack memory > FALSE - Don't care of protection of stack memory > PcdImageProtectionPolicy > 1 - Apply NX to data section of image from the corresponding origin > 0 - Don't care of the protection of data section of image from the > corresponding origin > PcdDxeNxMemoryProtectionPolicy > 1 - Apply NX to corresponding type of memory > 0 - Don't care of the protection of corresponding type of memory Good summary, and I think this is a workable approach as well. Defining value 0 as "don't care" or "no-op", rather than "unset this feature", eliminates contradictory configurations. Thanks Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 0/7] MdeModulePkg: add support for dispatching foreign arch PE/COFF images
On 19 September 2018 at 02:29, Yao, Jiewen wrote: > Thank you Ard. Good to know. > > Did you also try some security test, such as input a bad image to see if the > code can return failure gracefully? > > Or enable secure boot to see if the image verification process still works > well ? > > One more, did you enable tpm to see if tpm measurement still works well ? > > Also did defer image solution still takes effect with this change? > > Sorry to ask many questions, I want to make sure the current security design > still work with this new capability. > Hello Jiewen, As far as I can tell, all the security checks are done *before* CoreLoadPeImage() is called, and the code flow has not changed at all before that point. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 0/7] MdeModulePkg: add support for dispatching foreign arch PE/COFF images
Ok. Cool! > -Original Message- > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] > Sent: Wednesday, September 19, 2018 9:55 PM > To: Yao, Jiewen > Cc: Ni, Ruiyu ; Zimmer, Vincent > ; Dong, Eric ; > edk2-devel@lists.01.org; Andrew Fish ; Gao, Liming > ; Kinney, Michael D ; > Richardson, Brian ; Carsey, Jaben > ; Zeng, Star > Subject: Re: [edk2] [PATCH v2 0/7] MdeModulePkg: add support for > dispatching foreign arch PE/COFF images > > On 19 September 2018 at 02:29, Yao, Jiewen > wrote: > > Thank you Ard. Good to know. > > > > Did you also try some security test, such as input a bad image to see if the > code can return failure gracefully? > > > > Or enable secure boot to see if the image verification process still works > well ? > > > > One more, did you enable tpm to see if tpm measurement still works well ? > > > > Also did defer image solution still takes effect with this change? > > > > Sorry to ask many questions, I want to make sure the current security > design still work with this new capability. > > > > Hello Jiewen, > > As far as I can tell, all the security checks are done *before* > CoreLoadPeImage() is called, and the code flow has not changed at all > before that point. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] SPI Flash Corruption
Hello, Our product uses a Baytrail with Minnowboard Max bios firmware ( version 0.93). Every now and then we see SPI flash corruption due to power cuts while the unit is booting which causes the unit not to boot anymore. After investigation we noticed that the VPD area is all FFs (address 44000->47DFF0). We have noticed that the Bios while booting writes to the flash from several places in the code, which is if interrupted most probably is causing the corruption. Why is the bios writing all these configurations to flash while booting, is it to optimize boot time? is it ok if we disable the bios writing to flash completely to protect ourselves from corruption? Samah ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Update SmiEntry function run the same position
Laszlo: I understand your point. I agree your suggestion. BZ https://bugzilla.tianocore.org/show_bug.cgi?id=1191 has been submitted. Yes. PiSmmCpuSmiEntryFixupAddress() is called in the driver entry point to fix up the address first. I will send V2 patch with the detail commit message and code comments. > -Original Message- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Thursday, September 13, 2018 3:49 AM > To: Gao, Liming > Cc: edk2-devel@lists.01.org; Yao, Jiewen ; Dong, Eric > ; Kinney, Michael D > ; Leif Lindholm (Linaro address) > ; Ard Biesheuvel ; > Andrew Fish ; Cetola, Stephano > Subject: Re: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Update SmiEntry > function run the same position > > On 09/12/18 17:42, Gao, Liming wrote: > > Laszlo: > > Before commit e21e355e2ca7fefb15b4df7078f995d3fb9c2b89, jmp _SmiHandler > > is commented. And below code, > ASM_PFX(CpuSmmDebugEntry) is moved into rax, then call it. But, this code > doesn't work in XCODE5 tool chain. Like you say, XCODE5 > doesn't generated the absolute address in the EFI image. So, rax stores the > relative address. Once this logic is moved to another place, it > will not work. > > ; jmp _SmiHandler ; instruction is not needed > > ... > > mov rax, ASM_PFX(CpuSmmDebugEntry) > > callrax > > > > Commit e21e355e2ca7fefb15b4df7078f995d3fb9c2b89 is to support XCODE5. I > > choose one tricky way to fix it. Although SmiEntry > logic is copied to another place and run, but here I enable jmp _SmiHandler > to jmp the original code location, then call > ASM_PFX(CpuSmmDebugEntry) with the relative address can work. > > mov rax, strict qword 0 ; mov rax, _SmiHandler > > _SmiHandlerAbsAddr: > > jmp rax > > ... > > callASM_PFX(CpuSmmDebugEntry) > > > > Today, Jiewen raises the issue that SmiHandler should run in the copied > > address, can't run in the common address. So, I update its > logic and remove jmp _SmiHandler, then keep code continue run in copied > address. But, I still need to fix up CpuSmmDebugEntry > address to the absolute address. They are both for this issue. They can't be > separated. > > > > ... > > mov rax, strict qword 0 ; call > > ASM_PFX(CpuSmmDebugEntry) > > CpuSmmDebugEntryAbsAddr: > > callrax > > Thank you very much for the explanation. I understand now. > > I also understand why I got confused so much earlier. The reason is that > the code was not commented sufficiently. It should have been pointed out > what part of the instruction stream was meant to be executed from the > copied address space (that is, not from the PiSmmCpuDxeSmm image > itself), and what part of the instruction stream was meant to execute > from within the the PiSmmCpuDxeSmm image. Without such comments, it's > too hard to understand the transitions. > > You or Jiewen should please file a TianoCore BZ describing the current > issue. Namely that, due to circumstances you are not allowed to share, > no part of the _SmiHandler routine should execute from within the > PiSmmCpuDxeSmm image; instead, all of it should execute from the > copied-to address space. This requires eliminating the jump back into > the PiSmmCpuDxeSmm image, and also patching up the relative addresses > (to absolute addresses), for those instructions that used to run from > within the PiSmmCpuDxeSmm image, but now no longer will. The necessary > changes should not affect the behavior of platforms that already consume > PiSmmCpuDxeSmm from edk2. > > The historical overview you provide above is also valuable, please > include it in the commit message and/or the BZ as well. > > It would still be nice to comment what parts of the source file run from > within the PiSmmCpuDxeSmm image. For example, > PiSmmCpuSmiEntryFixupAddress() does, right? > > --*-- > > Please understand that my issue here is more serious than just missing > the explanation / motivation, for this specific patch. My issue is that > the posting of this work to edk2-devel should have *started* with the > above explanation. You and Jiewen understand the issue. Noone else on > the list (that doesn't work at your office) does. I've wasted half a day > because you didn't write up the background up-front. > > I don't need to know your specific internal proprietary feature that > requires this change. I do need to know that an internal feature with > such a requirement exists, and that it is your motivation. Every ten > minutes you save for yourselves on documentation is amplified to several > hours of struggle, for each reviewer. And we specifically discussed this > scenario with Mike at one of the earlier stewards' meetings. > > (The general topic back then was my raising that you [plural] liberally > commit whatever you want to MdePkg / MdeModulePkg, without really naming > the use cases, let alone adding client code to edk2. Whereas non-Intel > contributors with a demonstrat
Re: [edk2] Updating/adding video mode
Prabin, There is not an easy answer to your question. 1) What video resolution is available can be a function of what monitor is plugged in to the graphics card. 2) The monitor can publish an EDID that defines what resolutions the monitor supports. EDID is a VESA standard. 3) An EFI Platform can provide an EFI_EDID_OVERRIDE_PROTOCOL that can impact the available video modes that get published. 4) The GOP card will publish EDID information via the EFI_EDID_DISCOVERED_PROTOCOL. As I mentioned the EDID data structure is defined by a VESA standard, but it also has to be valid for what the monitor can support. Basically you can override the EDID and tell the GOP card to does something the monitor can not support. Some platforms use the ConSpliter and it produces a virtual GOP protocol that will aggregate hardware GOPs. So for example if you system has an internal panel and an external monitor installed you could end up with 2 HW GOP protocols and a virtual GOP protocol that represents the ConSpliter. Thanks, Andrew Fish > On Sep 15, 2018, at 10:37 PM, prabin ca wrote: > > Any points on this really helpful for me. > >> On 15-Sep-2018, at 6:46 AM, prabin ca wrote: >> >> Hi Team, >> I’m working with a platform having UHD (3840X2160as native resolution) >> display screen. I have dump the video modes using EFI_GRAPHICS_PROTOCOL. >> >> Following are the dump result >> >> Mode 0 : hRes = 3840 Vres=2160 >> Mode 1 : hRes = 640 Vres = 480 >> Mode 2 : hRes = 800 Vres = 600 >> Mode 3 : hRes = 1024 Vres = 768 >> Mode 4 : hRes = 1280 Vres = 1024 >> Mode 5 : hRes = 1600 Vres = 1200 >> Mode 6 : hRes = 1920 Vres = 1440 >> >> In the supported video mode list, 1920X1080 is not there. Is there any way >> to add support for this resolution. I have checked UEFI spec but didn’t find >> any useful API/protocol >> Any help really appreciate. > ___ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 0/4] MdeModulePkg: add support for dispatching foreign arch PE/COFF images
> On Sep 15, 2018, at 6:28 AM, Ard Biesheuvel wrote: > > On 13 September 2018 at 19:20, Kinney, Michael D > wrote: >> Ard, >> >> I think there is a fundamental assumption that >> the sizeof(UINTN) and size of pointers of >> the native CPU are the same as the emulated CPU. >> If that is not the case, then I would like to see >> more details. Otherwise that is a significant >> restriction that needs to be clearly documented. >> > > There is no such assumption. The PE/COFF emulator protocol is an > abstract protocol that leaves it fully up to the implementation to > decide whether it can support images of machine type X and image type > Y. > Ard, Not knowing the size of UINTN or a pointer was very painful in terms of how EBC worked. The compiler was forced to generate code for sizeof vs. resolving it at compile time. >> Protocols that only allow a single instance need to >> clearly document that assumption. >> > > I will remove that restriction. > >> If we decide to treat EBC as an emulated CPU, then >> we would want to support multiple instances of the >> protocol. The updates to the DXE Core are a bit >> confusing because it has separate handling of EBC >> and emulated CPUs. I think it would make the DXE >> Core logic simpler and easier to understand if they >> were combined. >> > > Yes, excellent idea, and it results in a nice cleanup as well > >> I asked about the startup case because if we do EBC, >> then we may need more services in the protocol because >> of the thunk code between native and EBC code. At the >> time EBC was originally implemented, we did not have >> paging enabled and the EBC interpreter work without >> depending on a page fault handler. >> >> The way the protocol is currently defined, I believe it >> fundamentally assumes paging is enabled. If paging is >> not enabled, then the current protocol services are not >> sufficient for any emulated CPU. Do we want this to work >> for paging disabled cases? If not, another assumption >> to clearly document. >> > > The paging disabled case is interesting. Does the UEFI spec even > permit running in DXE with paging disabled? Paging is a function of the processor binding in the UEFI spec. It is not required for IA32. For X64 you have to have paging enable to enter long mode. On other processors you need to turn on paging to control the cache. So I guess the no paging is likely more of a i386 issue? > > In any case, I will send out a v2 as the basis for further discussion. > We can also sit down and discuss it in Vancouver the coming week. I'd suggest passing an EFI device path to IS_PECOFF_IMAGE_SUPPORTED. At some point it might be useful for the emulator to know what device is being emulated. For bonus points we could check IsPecoffImageSupported() prior to the native check. Never say never, some one may want to have emulator run on native images for debugging etc. Thanks, Andrew Fish > ___ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 0/4] MdeModulePkg: add support for dispatching foreign arch PE/COFF images
On 19 September 2018 at 12:35, Andrew Fish wrote: > > >> On Sep 15, 2018, at 6:28 AM, Ard Biesheuvel >> wrote: >> >> On 13 September 2018 at 19:20, Kinney, Michael D >> wrote: >>> Ard, >>> >>> I think there is a fundamental assumption that >>> the sizeof(UINTN) and size of pointers of >>> the native CPU are the same as the emulated CPU. >>> If that is not the case, then I would like to see >>> more details. Otherwise that is a significant >>> restriction that needs to be clearly documented. >>> >> >> There is no such assumption. The PE/COFF emulator protocol is an >> abstract protocol that leaves it fully up to the implementation to >> decide whether it can support images of machine type X and image type >> Y. >> > > Ard, > > Not knowing the size of UINTN or a pointer was very painful in terms of how > EBC worked. The compiler was forced to generate code for sizeof vs. resolving > it at compile time. > Oh, I'm sure - but my point is that whether architecture X can be emulated on architecture Y and how is a limitation that affects some implementations of the protocol, but at the abstract level that we deal with in the core code, it is not a concern. >>> Protocols that only allow a single instance need to >>> clearly document that assumption. >>> >> >> I will remove that restriction. >> >>> If we decide to treat EBC as an emulated CPU, then >>> we would want to support multiple instances of the >>> protocol. The updates to the DXE Core are a bit >>> confusing because it has separate handling of EBC >>> and emulated CPUs. I think it would make the DXE >>> Core logic simpler and easier to understand if they >>> were combined. >>> >> >> Yes, excellent idea, and it results in a nice cleanup as well >> >>> I asked about the startup case because if we do EBC, >>> then we may need more services in the protocol because >>> of the thunk code between native and EBC code. At the >>> time EBC was originally implemented, we did not have >>> paging enabled and the EBC interpreter work without >>> depending on a page fault handler. >>> >>> The way the protocol is currently defined, I believe it >>> fundamentally assumes paging is enabled. If paging is >>> not enabled, then the current protocol services are not >>> sufficient for any emulated CPU. Do we want this to work >>> for paging disabled cases? If not, another assumption >>> to clearly document. >>> >> >> The paging disabled case is interesting. Does the UEFI spec even >> permit running in DXE with paging disabled? > > Paging is a function of the processor binding in the UEFI spec. It is not > required for IA32. For X64 you have to have paging enable to enter long mode. > On other processors you need to turn on paging to control the cache. So I > guess the no paging is likely more of a i386 issue? > Michael spotted yesterday that RISC-V mandates paging disabled, which is peculiar in itself. But again, whether a certain implementation of the protocol relies on paging or not is an implementation detail. >> >> In any case, I will send out a v2 as the basis for further discussion. >> We can also sit down and discuss it in Vancouver the coming week. > > I'd suggest passing an EFI device path to IS_PECOFF_IMAGE_SUPPORTED. At some > point it might be useful for the emulator to know what device is being > emulated. > I guess you mean for which device we are loading a driver that relies on emulation? I guess that makes sense for option ROMs, which is the primary use case, so yes, I can add that. > For bonus points we could check IsPecoffImageSupported() prior to the native > check. Never say never, some one may want to have emulator run on native > images for debugging etc. > Peter brought this up as well - in some cases, you may want to sandbox X86 drivers running on X86 by running them on an emulator. If you think the overhead of performing this check for each image rather than only for images that have already been found to be for a foreign architecture is acceptables then I'm happy to change this. But we can easily do that later as well. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 3/3] SecurityPkg: remove PE/COFF header workaround for ELILO on IPF
On 7 September 2018 at 01:28, Laszlo Ersek wrote: > On 09/07/18 07:42, Ard Biesheuvel wrote: >> Now that Itanium support has been dropped, we can remove the various >> occurrences of the ELILO on Itanium PE/COFF header workaround. >> >> Link: https://bugzilla.tianocore.org/show_bug.cgi?id=816 >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Ard Biesheuvel >> --- >> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >> | 47 >> SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c >> | 27 +++ >> SecurityPkg/Tcg/Tcg2Dxe/MeasureBootPeCoff.c >> | 27 +++ >> >> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c >> | 25 +++ >> 4 files changed, 25 insertions(+), 101 deletions(-) > > Reviewed-by: Laszlo Ersek > Chao, Jiewen: any concerns? ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] Stack issue after warm UEFI reset and MMU enabling on an Armv8 platform
Hi All,
I need UEFI experts help on the problem with Armv8 board on warm UEFI reset.
Cold reset works fine.
Here is how I set up a warm reset:
STATIC
EFI_STATUS
ShutdownUefiBootServices (
VOID
)
{
EFI_STATUS Status;
UINTN MemoryMapSize;
EFI_MEMORY_DESCRIPTOR *MemoryMap;
UINTN MapKey;
UINTN DescriptorSize;
UINT32 DescriptorVersion;
UINTN Pages;
MemoryMap = NULL;
MemoryMapSize = 0;
Pages = 0;
do {
Status = gBS->GetMemoryMap (
&MemoryMapSize,
MemoryMap,
&MapKey,
&DescriptorSize,
&DescriptorVersion
);
if (Status == EFI_BUFFER_TOO_SMALL) {
Pages = EFI_SIZE_TO_PAGES (MemoryMapSize) + 1;
MemoryMap = AllocatePages (Pages);
//
// Get System MemoryMap
//
Status = gBS->GetMemoryMap (
&MemoryMapSize,
MemoryMap,
&MapKey,
&DescriptorSize,
&DescriptorVersion
);
}
// Don't do anything between the GetMemoryMap() and ExitBootServices()
if (!EFI_ERROR(Status)) {
Status = gBS->ExitBootServices (gImageHandle, MapKey);
if (EFI_ERROR(Status)) {
FreePages (MemoryMap, Pages);
MemoryMap = NULL;
MemoryMapSize = 0;
}
}
} while (EFI_ERROR(Status));
return Status;
}
Then perform
ArmCleanDataCache ();
ArmInvalidateDataCache ();
ArmDisableInstructionCache ();
ArmInvalidateInstructionCache ();
ArmDisableMmu ();
Then jump to start of FV:
typedef
VOID
(EFIAPI *START_FV)(
VOID
);
StartOfFv = (START_FV)(UINTN)PcdGet64(PcdFvBaseAddress);
StartOfFv ();
Now this is what happens on warm reset:
reset -c warm
1. Until ArmEnableMmu() gets called, everything works as expected.
Here is the stack right before ArmEnableMmu() is called:
ArmConfigureMmu+0x4f8
InitMmu+0x24
MemoryPeim+0x440
PrePiMain+0x114
PrimaryMain+0x68
CEntryPoint+0xC4
EL2:0x88BC
- End of stack info -
2. Here is the stack as soon as Mmu is enabled with ArmEnableMmu() :
ArmConfigureMmu+0x4fc <-- This one is correct, at line 745 in
ArmConfigureMmu() in ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
(return EFI_SUCCESS)
_ModuleEntryPoint+0x24 <-- Wrong. This points directly to ASSERT(FALSE);
and to CpuDeadLoop() in DxeCoreEntryPoint.c, lines 59-60.
El2:0x8E5E8300 <-- Absolutely bogus
--- End of stack info ---
So, as soon as ArmEnableMmu() exits, execution jumps directly to
CpuDeadLoop() in DxeCoreEntryPoint of _ModuleEntryPoint().
Would be grateful for any advice.
Thank you,
Vladimir
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] Stack issue after warm UEFI reset and MMU enabling on an Armv8 platform
On 19 September 2018 at 15:55, Vladimir Olovyannikov <
vladimir.olovyanni...@broadcom.com> wrote:
> Hi All,
>
>
>
> I need UEFI experts help on the problem with Armv8 board on warm UEFI
> reset.
>
> Cold reset works fine.
>
>
>
> Here is how I set up a warm reset:
>
>
>
> STATIC
>
> EFI_STATUS
>
> ShutdownUefiBootServices (
>
> VOID
>
> )
>
> {
>
> EFI_STATUS Status;
>
> UINTN MemoryMapSize;
>
> EFI_MEMORY_DESCRIPTOR *MemoryMap;
>
> UINTN MapKey;
>
> UINTN DescriptorSize;
>
> UINT32 DescriptorVersion;
>
> UINTN Pages;
>
>
>
> MemoryMap = NULL;
>
> MemoryMapSize = 0;
>
> Pages = 0;
>
>
>
> do {
>
> Status = gBS->GetMemoryMap (
>
> &MemoryMapSize,
>
> MemoryMap,
>
> &MapKey,
>
> &DescriptorSize,
>
> &DescriptorVersion
>
> );
>
> if (Status == EFI_BUFFER_TOO_SMALL) {
>
>
>
> Pages = EFI_SIZE_TO_PAGES (MemoryMapSize) + 1;
>
> MemoryMap = AllocatePages (Pages);
>
>
>
> //
>
> // Get System MemoryMap
>
> //
>
> Status = gBS->GetMemoryMap (
>
> &MemoryMapSize,
>
> MemoryMap,
>
> &MapKey,
>
> &DescriptorSize,
>
> &DescriptorVersion
>
> );
>
> }
>
>
>
> // Don't do anything between the GetMemoryMap() and ExitBootServices()
>
> if (!EFI_ERROR(Status)) {
>
> Status = gBS->ExitBootServices (gImageHandle, MapKey);
>
> if (EFI_ERROR(Status)) {
>
> FreePages (MemoryMap, Pages);
>
> MemoryMap = NULL;
>
> MemoryMapSize = 0;
>
> }
>
> }
>
> } while (EFI_ERROR(Status));
>
>
>
> return Status;
>
> }
>
>
>
> Then perform
>
> ArmCleanDataCache ();
>
> ArmInvalidateDataCache ();
>
> ArmDisableInstructionCache ();
>
> ArmInvalidateInstructionCache ();
>
These don't do anything useful on ARM. You can only reliably perform cache
maintenance by virtual address.
ArmDisableMmu ();
>
>
>
... so after this call returns, all bets are off with regards to whether
what is popped from the stack is actually what we pushed when we entered
the function.
> Then jump to start of FV:
>
>
>
> typedef
>
> VOID
>
> (EFIAPI *START_FV)(
>
> VOID
>
> );
>
> StartOfFv = (START_FV)(UINTN)PcdGet64(PcdFvBaseAddress);
>
> StartOfFv ();
>
>
>
> Now this is what happens on warm reset:
>
> reset -c warm
>
> 1. Until ArmEnableMmu() gets called, everything works as expected.
>
> Here is the stack right before ArmEnableMmu() is called:
>
> ArmConfigureMmu+0x4f8
>
> InitMmu+0x24
>
> MemoryPeim+0x440
>
> PrePiMain+0x114
>
> PrimaryMain+0x68
>
> CEntryPoint+0xC4
>
> EL2:0x88BC
>
> - End of stack info -
>
>
>
> 2. Here is the stack as soon as Mmu is enabled with ArmEnableMmu() :
>
> ArmConfigureMmu+0x4fc <-- This one is correct, at line 745 in
> ArmConfigureMmu() in ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> (return EFI_SUCCESS)
>
>_ModuleEntryPoint+0x24 <-- Wrong. This points directly to
> ASSERT(FALSE); and to CpuDeadLoop() in DxeCoreEntryPoint.c, lines 59-60.
>
>El2:0x8E5E8300 <-- Absolutely bogus
>
> --- End of stack info ---
>
>
>
> So, as soon as ArmEnableMmu() exits, execution jumps directly to
> CpuDeadLoop() in DxeCoreEntryPoint of _ModuleEntryPoint().
>
>
>
> Would be grateful for any advice.
>
>
>
> Thank you,
>
> Vladimir
>
>
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] Stack issue after warm UEFI reset and MMU enabling on an Armv8 platform
>From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>Sent: Wednesday, September 19, 2018 4:38 PM
>To: Vladimir Olovyannikov
>Cc: edk2-devel@lists.01.org
>Subject: Re: Stack issue after warm UEFI reset and MMU enabling on an Armv8
>platform
>On 19 September 2018 at 15:55, Vladimir Olovyannikov
> wrote:
>>Hi All,
>>I need UEFI experts help on the problem with Armv8 board on warm UEFI
>>reset.
>>Cold reset works fine.
>>Here is how I set up a warm reset:
>>STATIC
>>EFI_STATUS
>>ShutdownUefiBootServices (
>> VOID
>> )
>>{
>> EFI_STATUS Status;
>> UINTN MemoryMapSize;
>> EFI_MEMORY_DESCRIPTOR *MemoryMap;
>> UINTN MapKey;
>> UINTN DescriptorSize;
>> UINT32 DescriptorVersion;
>> UINTN Pages;
>> MemoryMap = NULL;
>> MemoryMapSize = 0;
>> Pages = 0;
>>
>> do {
>>Status = gBS->GetMemoryMap (
>>&MemoryMapSize,
>>MemoryMap,
>>&MapKey,
>>&DescriptorSize,
>>&DescriptorVersion
>>);
>>if (Status == EFI_BUFFER_TOO_SMALL) {
>>
>> Pages = EFI_SIZE_TO_PAGES (MemoryMapSize) + 1;
>> MemoryMap = AllocatePages (Pages);
>>
>> //
>> // Get System MemoryMap
>> //
>> Status = gBS->GetMemoryMap (
>> &MemoryMapSize,
>> MemoryMap,
>> &MapKey,
>> &DescriptorSize,
>> &DescriptorVersion
>> );
>>}
>>
>>// Don't do anything between the GetMemoryMap() and ExitBootServices()
>>if (!EFI_ERROR(Status)) {
>> Status = gBS->ExitBootServices (gImageHandle, MapKey);
>> if (EFI_ERROR(Status)) {
>>FreePages (MemoryMap, Pages);
>>MemoryMap = NULL;
>>MemoryMapSize = 0;
>> }
>>}
>> } while (EFI_ERROR(Status));
>>
>> return Status;
>>}
>>
>>Then perform
>>ArmCleanDataCache ();
>>ArmInvalidateDataCache ();
>>ArmDisableInstructionCache ();
>>ArmInvalidateInstructionCache ();
>These don't do anything useful on ARM. You can only reliably perform cache
>maintenance by virtual address.
So, should I just remove them altogether?
>>ArmDisableMmu ();
>... so after this call returns, all bets are off with regards to whether
>what is popped from the stack is actually what we pushed when we entered
>the function.
OK, thank you for explanation.
But this call returns back into ResetLib implementation as it should, and
then there is a direct jump to the start of FV.
Am I doing anything wrong here?
Then, up to the point of enabling of MMU the stack is OK. But right after
enabling MMU it points at _ModuleEntryPoint end of function in
DxeCoreEntryPoint.c
Am I missing anything? Maybe some stack cleanup before jumping to the start
of FV?
>>Then jump to start of FV:
>>typedef
>>VOID
>> (EFIAPI *START_FV)(
>> VOID
>>);
>>StartOfFv = (START_FV)(UINTN)PcdGet64(PcdFvBaseAddress);
>>StartOfFv ();
>>
>>Now this is what happens on warm reset:
>>reset -c warm
>>1. Until ArmEnableMmu() gets called, everything works as expected.
>>Here is the stack right before ArmEnableMmu() is called:
>> ArmConfigureMmu+0x4f8
>> InitMmu+0x24
>> MemoryPeim+0x440
>> PrePiMain+0x114
>> PrimaryMain+0x68
>> CEntryPoint+0xC4
>> EL2:0x88BC
>> - End of stack info -
>>
>>2. Here is the stack as soon as Mmu is enabled with ArmEnableMmu() :
>>ArmConfigureMmu+0x4fc <-- This one is correct, at line 745 in
>> ArmConfigureMmu() in ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> (return EFI_SUCCESS)
>> _ModuleEntryPoint+0x24 <-- Wrong. This points directly to
>> ASSERT(FALSE); and to CpuDeadLoop() in DxeCoreEntryPoint.c, lines 59-60.
>> El2:0x8E5E8300 <-- Absolutely bogus
>>--- End of stack info ---
>>
>>So, as soon as ArmEnableMmu() exits, execution jumps directly to
>>CpuDeadLoop() in DxeCoreEntryPoint of _ModuleEntryPoint().
>>
>>Would be grateful for any advice.
>>
>>Thank you,
>>Vladimir
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] Stack issue after warm UEFI reset and MMU enabling on an Armv8 platform
On 19 September 2018 at 16:58, Vladimir Olovyannikov <
vladimir.olovyanni...@broadcom.com> wrote:
> >From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> >Sent: Wednesday, September 19, 2018 4:38 PM
> >To: Vladimir Olovyannikov
> >Cc: edk2-devel@lists.01.org
> >Subject: Re: Stack issue after warm UEFI reset and MMU enabling on an
> Armv8
> >platform
>
>
> >On 19 September 2018 at 15:55, Vladimir Olovyannikov
> > wrote:
> >>Hi All,
>
> >>I need UEFI experts help on the problem with Armv8 board on warm UEFI
> >>reset.
> >>Cold reset works fine.
>
> >>Here is how I set up a warm reset:
>
> >>STATIC
> >>EFI_STATUS
> >>ShutdownUefiBootServices (
> >> VOID
> >> )
> >>{
> >> EFI_STATUS Status;
> >> UINTN MemoryMapSize;
> >> EFI_MEMORY_DESCRIPTOR *MemoryMap;
> >> UINTN MapKey;
> >> UINTN DescriptorSize;
> >> UINT32 DescriptorVersion;
> >> UINTN Pages;
>
> >> MemoryMap = NULL;
> >> MemoryMapSize = 0;
> >> Pages = 0;
> >>
> >> do {
> >>Status = gBS->GetMemoryMap (
> >>&MemoryMapSize,
> >>MemoryMap,
> >>&MapKey,
> >>&DescriptorSize,
> >>&DescriptorVersion
> >>);
> >>if (Status == EFI_BUFFER_TOO_SMALL) {
> >>
> >> Pages = EFI_SIZE_TO_PAGES (MemoryMapSize) + 1;
> >> MemoryMap = AllocatePages (Pages);
> >>
> >> //
> >> // Get System MemoryMap
> >> //
> >> Status = gBS->GetMemoryMap (
> >> &MemoryMapSize,
> >> MemoryMap,
> >> &MapKey,
> >> &DescriptorSize,
> >> &DescriptorVersion
> >> );
> >>}
> >>
> >>// Don't do anything between the GetMemoryMap() and
> ExitBootServices()
> >>if (!EFI_ERROR(Status)) {
> >> Status = gBS->ExitBootServices (gImageHandle, MapKey);
> >> if (EFI_ERROR(Status)) {
> >>FreePages (MemoryMap, Pages);
> >>MemoryMap = NULL;
> >>MemoryMapSize = 0;
> >> }
> >>}
> >> } while (EFI_ERROR(Status));
> >>
> >> return Status;
> >>}
> >>
> >>Then perform
> >>ArmCleanDataCache ();
> >>ArmInvalidateDataCache ();
> >>ArmDisableInstructionCache ();
> >>ArmInvalidateInstructionCache ();
>
> >These don't do anything useful on ARM. You can only reliably perform cache
> >maintenance by virtual address.
> So, should I just remove them altogether?
>
> >>ArmDisableMmu ();
>
> >... so after this call returns, all bets are off with regards to whether
> >what is popped from the stack is actually what we pushed when we entered
> >the function.
> OK, thank you for explanation.
> But this call returns back into ResetLib implementation as it should, and
> then there is a direct jump to the start of FV.
> Am I doing anything wrong here?
> Then, up to the point of enabling of MMU the stack is OK. But right after
> enabling MMU it points at _ModuleEntryPoint end of function in
> DxeCoreEntryPoint.c
> Am I missing anything? Maybe some stack cleanup before jumping to the start
> of FV?
>
>
I guess you should be disabling interrupts as well. And quiesce all DMA
capable devices like network controllers that may corrupt your memory.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] Stack issue after warm UEFI reset and MMU enabling on an Armv8 platform
Of all the gin joints in all the towns in all the world, Vladimir Olovyannikov
had to walk into mine at 16:58 on Wednesday 19 September 2018 and say:
> >From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> >Sent: Wednesday, September 19, 2018 4:38 PM
> >To: Vladimir Olovyannikov
> >Cc: edk2-devel@lists.01.org
> >Subject: Re: Stack issue after warm UEFI reset and MMU enabling on an
> >Armv8 platform
> >
> >
> >On 19 September 2018 at 15:55, Vladimir Olovyannikov
> >
> > wrote:
> >>Hi All,
> >>
> >>I need UEFI experts help on the problem with Armv8 board on warm UEFI
> >>reset.
> >>Cold reset works fine.
> >>
> >>Here is how I set up a warm reset:
> >>
> >>STATIC
> >>EFI_STATUS
> >>ShutdownUefiBootServices (
> >>
> >> VOID
> >> )
> >>
> >>{
> >>
> >> EFI_STATUS Status;
> >> UINTN MemoryMapSize;
> >> EFI_MEMORY_DESCRIPTOR *MemoryMap;
> >> UINTN MapKey;
> >> UINTN DescriptorSize;
> >> UINT32 DescriptorVersion;
> >> UINTN Pages;
> >>
> >> MemoryMap = NULL;
> >> MemoryMapSize = 0;
> >> Pages = 0;
> >>
> >> do {
> >>
> >>Status = gBS->GetMemoryMap (
> >>
> >>&MemoryMapSize,
> >>MemoryMap,
> >>&MapKey,
> >>&DescriptorSize,
> >>&DescriptorVersion
> >>);
> >>
> >>if (Status == EFI_BUFFER_TOO_SMALL) {
> >>
> >> Pages = EFI_SIZE_TO_PAGES (MemoryMapSize) + 1;
> >> MemoryMap = AllocatePages (Pages);
> >>
> >> //
> >> // Get System MemoryMap
> >> //
> >> Status = gBS->GetMemoryMap (
> >>
> >> &MemoryMapSize,
> >> MemoryMap,
> >> &MapKey,
> >> &DescriptorSize,
> >> &DescriptorVersion
> >> );
> >>
> >>}
> >>
> >>// Don't do anything between the GetMemoryMap() and
> >>ExitBootServices() if (!EFI_ERROR(Status)) {
> >>
> >> Status = gBS->ExitBootServices (gImageHandle, MapKey);
> >> if (EFI_ERROR(Status)) {
> >>
> >>FreePages (MemoryMap, Pages);
> >>MemoryMap = NULL;
> >>MemoryMapSize = 0;
> >>
> >> }
> >>
> >>}
> >>
> >> } while (EFI_ERROR(Status));
> >>
> >> return Status;
> >>
> >>}
> >>
> >>Then perform
> >>ArmCleanDataCache ();
> >>ArmInvalidateDataCache ();
> >>ArmDisableInstructionCache ();
> >>ArmInvalidateInstructionCache ();
> >
> >These don't do anything useful on ARM. You can only reliably perform cache
> >maintenance by virtual address.
>
> So, should I just remove them altogether?
>
> >>ArmDisableMmu ();
> >
> >... so after this call returns, all bets are off with regards to whether
> >what is popped from the stack is actually what we pushed when we entered
> >the function.
>
> OK, thank you for explanation.
> But this call returns back into ResetLib implementation as it should, and
> then there is a direct jump to the start of FV.
> Am I doing anything wrong here?
> Then, up to the point of enabling of MMU the stack is OK. But right after
> enabling MMU it points at _ModuleEntryPoint end of function in
> DxeCoreEntryPoint.c
> Am I missing anything? Maybe some stack cleanup before jumping to the start
> of FV?
When the MMU is enabled, does the mapping for the stack pages change? That is,
could the stack now be mapped to different physical page now?
Instead of showing a stack trace, can you dump the stack pages and compare the
before and after contents?
Assuming the same physical memory pages are still being used, then there could
be a cache flushing problem. What could happen is:
- some stack memory has been touched recently and is now in the data cache
- changes are made, which are written to the cache, but not yet flushed out to
RAM
- enabling the MMU causes a full invalidate of the cache
Now when you look at the stack, you see the earlier contents that were in RAM
-- the changes previously only written to the cache have been lost.
Enabling/disabling caches and MMU is always tricky. I can't say for sure, but
I wouldn't be surprised if there's some subtle bug that causes a flush
operation to be missed and things may just work by coincidence in the cold
start case.
-Bill
> >>Then jump to start of FV:
> >>
> >>typedef
> >>VOID
> >>
> >> (EFIAPI *START_FV)(
> >>
> >> VOID
> >>
> >>);
> >>StartOfFv = (START_FV)(UINTN)PcdGet64(PcdFvBaseAddress);
> >>StartOfFv ();
> >>
> >>Now this is what happens on warm reset:
> >>reset -c warm
> >>1. Until ArmEnableMmu() gets called, everything works as expected.
> >>
> >>Here is the stack right before ArmEnableMmu() is called:
> >> ArmConfigureMmu+0x4f8
> >> InitMmu+0x24
> >> MemoryPeim+0x440
> >> PrePiMain+0x114
> >> PrimaryMain+0x68
> >> CEntryPoint+0xC4
> >> EL2:0x88BC
> >> - End of sta
Re: [edk2] [PATCH 3/3] IntelFsp2Pkg: Tell git to format SplitFspBin.py with native newlines
Hi Patrick, > If the autoconversion is not considered good enough, I'd propose keeping out > patch 1 of this series that adds the #! line and the executable bit, and > instead expect people to always call the script with "python > $path/SplitFspBin.py" to keep confusion at a minimum. There was quite a debate on this a few years ago. Without getting into the details, the decision was made that people on Windows platforms would set core.autocrlf=false and that source code would be stored in CR-LF format. Per your recommendation, it sounds like we should only merge patch 2 then. Thanks, Nate ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 3/3] SecurityPkg: remove PE/COFF header workaround for ELILO on IPF
No concern at all. I have given R-B for the whole patch series. :-) > -Original Message- > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] > Sent: Thursday, September 20, 2018 5:47 AM > To: Laszlo Ersek > Cc: edk2-devel@lists.01.org; Zeng, Star ; Wang, Jian J > ; Kinney, Michael D ; > Gao, Liming ; Zhang, Chao B > ; Yao, Jiewen ; Leif > Lindholm > Subject: Re: [PATCH v2 3/3] SecurityPkg: remove PE/COFF header > workaround for ELILO on IPF > > On 7 September 2018 at 01:28, Laszlo Ersek wrote: > > On 09/07/18 07:42, Ard Biesheuvel wrote: > >> Now that Itanium support has been dropped, we can remove the various > >> occurrences of the ELILO on Itanium PE/COFF header workaround. > >> > >> Link: https://bugzilla.tianocore.org/show_bug.cgi?id=816 > >> Contributed-under: TianoCore Contribution Agreement 1.1 > >> Signed-off-by: Ard Biesheuvel > >> --- > >> > SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > | 47 > >> > SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c > | 27 +++ > >> SecurityPkg/Tcg/Tcg2Dxe/MeasureBootPeCoff.c > | 27 +++ > >> > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfi > gImpl.c | 25 +++ > >> 4 files changed, 25 insertions(+), 101 deletions(-) > > > > Reviewed-by: Laszlo Ersek > > > > Chao, Jiewen: any concerns? ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch 0/5] Support windowsize to benefit tftp/pxe download performance.
Hi Laszlo, I agree there is no document to describe the detailed difference against the overlapped network drivers the between NetworkPkg and MdeModulePkg (except IPv4/IPv6 support ). We only declared that those drivers should not be used at the same (https://github.com/tianocore/tianocore.github.io/wiki/NetworkPkg-Getting-Started-Guide). Actually, the problem you mentioned here only exists in ISCSI/TCP/PXE drivers - Tcp4Dxe VS TcpDxe, IScsiDxe VS IScsiDxe, UefiPxeBcDxe VS UefiPxeBcDxe. So, as you said, it's the time for us to add some declaration somewhere (inf or wiki) -- For those three drivers in MdeModulePkg, they will remain unchanged until there is any specific requirement that we need fix any issue. That will greatly reduce the effort to maintain/test those combine of two drivers. So, we don’t recommend to use those three drivers in MdeModulePkg because they might some issues, which has been fixed in the NetworkPkg drivers. If you agree, we will add some statement in the corresponding *.inf files. Thanks, Jiaxin > -Original Message- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Wednesday, September 19, 2018 7:25 PM > To: Wu, Jiaxin ; edk2-devel@lists.01.org > Cc: Ye, Ting ; Carsey, Jaben ; > Fu, Siyuan ; Shao, Ming > Subject: Re: [edk2] [Patch 0/5] Support windowsize to benefit tftp/pxe > download performance. > > On 09/19/18 04:20, Wu, Jiaxin wrote: > >> On 09/17/18 07:43, Jiaxin Wu wrote: > >>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=886 > >>> > >>> The series patches are to support the TFTP windowsize option described > in > >> RFC 7440. > >>> TFTP shell command and UEFI PXE driver will use the feature to benefit > the > >> download > >>> performance. > >> > >> I tested this series, between two virtual machines running on my laptop. > >> The TFTP server program that I used was "tftp-server-5.2-22.el7.x86_64". > >> The downloaded file was 478,150,656 bytes in size. I built OVMF with > >> NETWORK_IP6_ENABLE, so that the last patch would take effect for both > >> PXEv4 and PXEv6. > >> > >> Before the series: > >> - PXEv4: 75 seconds (~ 6225 KB/s) > >> - PXEv6: 100 seconds (~ 4669 KB/s) > >> > >> After the series: > >> - PXEv4: 48 seconds (~ 9728 KB/s) > >> - PXEv6: 60 seconds (~ 7782 KB/s) > >> > >> These measurements are very rough (I didn't run them multiple times > >> etc), but I think they are still quite good indicators. > >> > >> For the testing, I used the UEFI boot options in UiApp, and not the > >> shell command, hence I have no feedback on patch #3. > >> > >> For patches #1, #2, and #5: > >> > >> Tested-by: Laszlo Ersek > >> > > > > Thanks the verification. > > > > > >> However, as I pointed out elsewhere in the thread, I think: > >> > >> - You might want to port the changes from patch#5 to > >> "MdeModulePkg/Universal/Network/UefiPxeBcDxe" as well, in a > separate > >> patch (patch #6). > >> > >> - If not, then (a) we should document this feature difference in the INF > >> files of the UefiPxeBcDxe drivers, (b) patch #4 should be re-done so > >> that it target NetworkPkg, not MdeModulePkg. > >> > > > > As I said in the previous email, normally, we only add the new feature into > the combo driver. But I think that's depends on the request. If you want to > include the feature into MdeModulePkg/Universal/Network/UefiPxeBcDxe > since the OVMF platform might use it, I will create patch #6. If not, I will > follow the comments (a)/(b). > > I don't currently have a use case or "requirement" for the window size > feature to work in an OVMF build that does *not* have > NETWORK_IP6_ENABLE. So from my side, we can delay the porting until such > a need materializes (it might never materialize, of course!) > > However, because this information -- i.e., the feature separation > between the IPv4-only, and the combined IPv4/IPv6 driver -- is new to > me, can we please document it somewhere in the code, for example, in the > INF files? We don't have to spell out the TFTP window size feature by > name, just the fact that NetworkPkg's UefiPxeBcDxe has more features in > general (even such features that are orthogonal to internet protocol > version, v4 vs. v6). > > If such documentation already exists, then I missed it, sorry! > > Thanks! > Laszlo > > >> - Patch #4 (regardless of package DEC) should be extended with > >> documentation (both DEC and UNI). > >> > >> Thanks > >> Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v2 0/2] clarify NXE enabling logic
> v2 changes: >Incorporates review comments from Laszlo and Star. BZ#1116: https://bugzilla.tianocore.org/show_bug.cgi?id=1116 Test: a. try all related PCDs combinations and check the page table attributes b. boot to shell on real intel platform with valid PCD setting combinations (IA32/X64) c. boot to fedora26, ubuntu18.04, windows 7 and windows 10 on OVMF emulated platform (X64) Cc: Star Zeng Cc: Laszlo Ersek Cc: Ard Biesheuvel Cc: Ruiyu Ni Cc: Jiewen Yao Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Jian J Wang Jian J Wang (2): MdeModulePkg/MdeModulePkg.dec/.uni: clarify PCDs usage MdeModulePkg/DxeIpl: support more NX related PCDs MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 2 ++ MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c | 4 ++-- MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 30 +++- MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h | 24 +++ MdeModulePkg/MdeModulePkg.dec| 20 MdeModulePkg/MdeModulePkg.uni| 13 ++ 6 files changed, 81 insertions(+), 12 deletions(-) -- 2.16.2.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v2 2/2] MdeModulePkg/DxeIpl: support more NX related PCDs
> v2 changes:
>a. remove macros no longer needed
>b. remove DEBUG and ASSERT in ToEnableExecuteDisableFeature()
>c. change ToEnableExecuteDisableFeature to EnableNonExec
BZ#1116: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
Currently IA32_EFER.NXE is only set against PcdSetNxForStack. This
confuses developers because following two other PCDs also need NXE
to be set, but actually not.
PcdDxeNxMemoryProtectionPolicy
PcdImageProtectionPolicy
This patch solves this issue by adding logic to enable IA32_EFER.NXE
if any of those PCDs have anything enabled.
Cc: Star Zeng
Cc: Laszlo Ersek
Cc: Ard Biesheuvel
Cc: Ruiyu Ni
Cc: Jiewen Yao
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang
---
MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 2 ++
MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c | 4 ++--
MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 30 +++-
MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h | 24 +++
4 files changed, 57 insertions(+), 3 deletions(-)
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index fd82657404..068e700074 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -117,6 +117,8 @@
[Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ##
SOMETIMES_CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ##
SOMETIMES_CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy ##
SOMETIMES_CONSUMES
[Depex]
gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid
diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
index d28baa3615..ccd30f964b 100644
--- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
@@ -245,7 +245,7 @@ ToBuildPageTable (
return TRUE;
}
- if (PcdGetBool (PcdSetNxForStack) && IsExecuteDisableBitAvailable ()) {
+ if (EnableNonExec ()) {
return TRUE;
}
@@ -436,7 +436,7 @@ HandOffToDxeCore (
BuildPageTablesIa32Pae = ToBuildPageTable ();
if (BuildPageTablesIa32Pae) {
PageTables = Create4GPageTablesIa32Pae (BaseOfStack, STACK_SIZE);
- if (IsExecuteDisableBitAvailable ()) {
+ if (EnableNonExec ()) {
EnableExecuteDisableBit();
}
}
diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
index 496e219913..73b0f67c6b 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
@@ -106,6 +106,31 @@ IsNullDetectionEnabled (
return ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) != 0);
}
+/**
+ Check if Execute Disable Bit (IA32_EFER.NXE) should be enabled or not.
+
+ @retval TRUEIA32_EFER.NXE should be enabled.
+ @retval FALSE IA32_EFER.NXE should not be enabled.
+
+**/
+BOOLEAN
+EnableNonExec (
+ VOID
+ )
+{
+ if (!IsExecuteDisableBitAvailable ()) {
+return FALSE;
+ }
+
+ //
+ // XD flag (BIT63) in page table entry is only valid if IA32_EFER.NXE is set.
+ // Features controlled by Following PCDs need this feature to be enabled.
+ //
+ return (PcdGetBool (PcdSetNxForStack) ||
+ PcdGet64 (PcdDxeNxMemoryProtectionPolicy) != 0 ||
+ PcdGet32 (PcdImageProtectionPolicy) != 0);
+}
+
/**
Enable Execute Disable Bit.
@@ -755,7 +780,10 @@ CreateIdentityMappingPageTables (
//
EnablePageTableProtection ((UINTN)PageMap, TRUE);
- if (PcdGetBool (PcdSetNxForStack)) {
+ //
+ // Set IA32_EFER.NXE if necessary.
+ //
+ if (EnableNonExec ()) {
EnableExecuteDisableBit ();
}
diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
index 85457ff937..09085312aa 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
@@ -179,6 +179,30 @@ typedef struct {
UINTN FreePages;
} PAGE_TABLE_POOL;
+/**
+ Check if Execute Disable Bit (IA32_EFER.NXE) should be enabled or not.
+
+ @retval TRUEIA32_EFER.NXE should be enabled.
+ @retval FALSE IA32_EFER.NXE should not be enabled.
+
+**/
+BOOLEAN
+EnableNonExec (
+ VOID
+ );
+
+/**
+ The function will check if Execute Disable Bit is available.
+
+ @retval TRUE Execute Disable Bit is available.
+ @retval FALSE Execute Disable Bit is not available.
+
+**/
+BOOLEAN
+IsExecuteDisableBitAvailable (
+ VOID
+ );
+
/**
Enable Execute Disable Bit.
--
2.16.2.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v2 1/2] MdeModulePkg/MdeModulePkg.dec/.uni: clarify PCDs usage
> v2 changes: >Newly added patch to clarify PCDs usage. BZ#1116: https://bugzilla.tianocore.org/show_bug.cgi?id=1116 The usage of following PCDs described in MdeModulePkg.dec don't match the implementation exactly. This patch updates related description in both .dec and .uni files to avoid confusion in platform configuration. PcdSetNxForStack PcdImageProtectionPolicy PcdDxeNxMemoryProtectionPolicy The main change is at the statement on how to handle the FALSE or 0 setting value in those PCDs. Current statement says the implementation should unset or disable related features but in fact the related code just do nothing (leave it to AS-IS). That means the result might be disabled, or might be not. It depends on other features or platform policy. For example, if one don't want to enforce NX onto stack memory, he/she needs to set PcdSetNxForStack to FALSE as well as to clear BIT4 of PcdDxeNxMemoryProtectionPolicy. Cc: Star Zeng Cc: Laszlo Ersek Cc: Ard Biesheuvel Cc: Ruiyu Ni Cc: Jiewen Yao Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Jian J Wang --- MdeModulePkg/MdeModulePkg.dec | 20 +++- MdeModulePkg/MdeModulePkg.uni | 13 + 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index 74a699cbb7..6566b57fd4 100644 --- a/MdeModulePkg/MdeModulePkg.dec +++ b/MdeModulePkg/MdeModulePkg.dec @@ -1288,17 +1288,23 @@ ## Set image protection policy. The policy is bitwise. # If a bit is set, the image will be protected by DxeCore if it is aligned. # The code section becomes read-only, and the data section becomes non-executable. - # If a bit is clear, the image will not be protected. + # If a bit is clear, nothing will be done to image code/data sections. #BIT0 - Image from unknown device. #BIT1 - Image from firmware volume. + # + # Note: If a bit is cleared, the data section could be still non-executable if + # PcdDxeNxMemoryProtectionPolicy is enabled for EfiLoaderData, EfiBootServicesData + # and/or EfiRuntimeServicesData. + # # @Prompt Set image protection policy. # @ValidRange 0x8002 | 0x - 0x001F gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy|0x0002|UINT32|0x1047 ## Set DXE memory protection policy. The policy is bitwise. # If a bit is set, memory regions of the associated type will be mapped - # non-executable. - # + # non-executable. + # If a bit is cleared, nothing will be done to associated type of memory. + # # Below is bit mask for this PCD: (Order is same as UEFI spec) # EfiReservedMemoryType 0x0001 # EfiLoaderCode 0x0002 @@ -1890,8 +1896,12 @@ # For the DxeIpl and the DxeCore are both X64, set NX for stack feature also require PcdDxeIplBuildPageTables be TRUE. # For the DxeIpl and the DxeCore are both IA32 (PcdDxeIplSwitchToLongMode is FALSE), set NX for stack feature also require # IA32 PAE is supported and Execute Disable Bit is available. - # TRUE - to set NX for stack. - # FALSE - Not to set NX for stack. + # + # Note: If this PCD is set to FALSE, NX could be still applied to stack due to PcdDxeNxMemoryProtectionPolicy enabled for + # EfiBootServicesData. + # + # TRUE - Set NX for stack. + # FALSE - Do nothing for stack. # @Prompt Set NX for stack. gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE|BOOLEAN|0x0001006f diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni index 080b8a62c0..61befdc1e4 100644 --- a/MdeModulePkg/MdeModulePkg.uni +++ b/MdeModulePkg/MdeModulePkg.uni @@ -345,8 +345,9 @@ "For the DxeIpl and the DxeCore are both X64, set NX for stack feature also require PcdDxeIplBuildPageTables be TRUE." "For the DxeIpl and the DxeCore are both IA32 (PcdDxeIplSwitchToLongMode is FALSE), set NX for stack feature also require" "IA32 PAE is supported and Execute Disable Bit is available." - "TRUE - to set NX for stack." - "FALSE - Not to set NX for stack." + "Note: If this PCD is set to FALSE, NX could be still applied to stack due to PcdDxeNxMemoryProtectionPolicy enabled for EfiBootServicesData." + "TRUE - Set NX for stack." + "FALSE - Do nothing for stack." #string
[edk2] [PATCH v1 1/5] MdePkg/BaseLib: Add new LoadFence API
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1193
This commit will add a new BaseLib API LoadFence(). This API will perform
a serializing operation on all load-from-memory instructions that were
issued prior to the call of this function.
The purpose of adding this API is to mitigate of the [CVE-2017-5753]
Bounds Check Bypass issue when untrusted data are being processed within
SMM. More details can be referred at the 'Bounds check bypass mitigation'
section at the below link:
https://software.intel.com/security-software-guidance/insights/host-firmware-speculative-execution-side-channel-mitigation
Cc: Ard Biesheuvel
Cc: Laszlo Ersek
Cc: Jiewen Yao
Cc: Michael D Kinney
Cc: Liming Gao
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu
---
MdePkg/Include/Library/BaseLib.h | 12 +++
MdePkg/Library/BaseLib/Arm/LoadFence.c | 26 ++
MdePkg/Library/BaseLib/BaseLib.inf | 4 +++
MdePkg/Library/BaseLib/Ebc/CpuBreakpoint.c | 15 +++-
MdePkg/Library/BaseLib/Ia32/LoadFence.nasm | 37 +++
MdePkg/Library/BaseLib/X64/LoadFence.nasm | 38
6 files changed, 131 insertions(+), 1 deletion(-)
diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index 123ae19dc2..194726ca35 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -4939,6 +4939,18 @@ MemoryFence (
/**
+ Performs a serializing operation on all load-from-memory instructions that
+ were issued prior to the call of this function.
+
+**/
+VOID
+EFIAPI
+LoadFence (
+ VOID
+ );
+
+
+/**
Saves the current CPU context that can be restored with a call to LongJump()
and returns 0.
diff --git a/MdePkg/Library/BaseLib/Arm/LoadFence.c
b/MdePkg/Library/BaseLib/Arm/LoadFence.c
new file mode 100644
index 00..69f0c3a07e
--- /dev/null
+++ b/MdePkg/Library/BaseLib/Arm/LoadFence.c
@@ -0,0 +1,26 @@
+/** @file
+ LoadFence() function for ARM.
+
+ Copyright (C) 2018, Intel Corporation. All rights reserved.
+
+ This program and the accompanying materials are licensed and made available
+ under the terms and conditions of the BSD License which accompanies this
+ distribution. The full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php.
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+ WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+**/
+
+/**
+ Performs a serializing operation on all load-from-memory instructions that
+ were issued prior to the call of this function.
+
+**/
+VOID
+EFIAPI
+LoadFence (
+ VOID
+ )
+{
+}
diff --git a/MdePkg/Library/BaseLib/BaseLib.inf
b/MdePkg/Library/BaseLib/BaseLib.inf
index a1b5ec4b75..f028fbc75a 100644
--- a/MdePkg/Library/BaseLib/BaseLib.inf
+++ b/MdePkg/Library/BaseLib/BaseLib.inf
@@ -68,6 +68,7 @@
[Sources.Ia32]
Ia32/WriteTr.nasm
+ Ia32/LoadFence.nasm
Ia32/Wbinvd.c | MSFT
Ia32/WriteMm7.c | MSFT
@@ -346,6 +347,7 @@
X64/EnableCache.nasm
X64/DisableCache.nasm
X64/WriteTr.nasm
+ X64/LoadFence.nasm
X64/CpuBreakpoint.c | MSFT
X64/WriteMsr64.c | MSFT
@@ -580,6 +582,7 @@
[Sources.ARM]
Arm/InternalSwitchStack.c
Arm/Unaligned.c
+ Arm/LoadFence.c
Math64.c | RVCT
Math64.c | MSFT
@@ -613,6 +616,7 @@
[Sources.AARCH64]
Arm/InternalSwitchStack.c
Arm/Unaligned.c
+ Arm/LoadFence.c
Math64.c
AArch64/MemoryFence.S | GCC
diff --git a/MdePkg/Library/BaseLib/Ebc/CpuBreakpoint.c
b/MdePkg/Library/BaseLib/Ebc/CpuBreakpoint.c
index 9b7d875664..a79461cfbf 100644
--- a/MdePkg/Library/BaseLib/Ebc/CpuBreakpoint.c
+++ b/MdePkg/Library/BaseLib/Ebc/CpuBreakpoint.c
@@ -1,7 +1,7 @@
/** @file
Base Library CPU Functions for EBC
- Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.
+ Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD
License
which accompanies this distribution. The full text of the license may be
found at
@@ -52,6 +52,19 @@ MemoryFence (
}
/**
+ Performs a serializing operation on all load-from-memory instructions that
+ were issued prior to the call of this function.
+
+**/
+VOID
+EFIAPI
+LoadFence (
+ VOID
+ )
+{
+}
+
+/**
Disables CPU interrupts.
**/
diff --git a/MdePkg/Library/BaseLib/Ia32/LoadFence.nasm
b/MdePkg/Library/BaseLib/Ia32/LoadFence.nasm
new file mode 100644
index 00..11600bea76
--- /dev/null
+++ b/MdePkg/Library/BaseLib/Ia32/LoadFence.nasm
@@ -0,0 +1,37 @@
+;--
;
+; Copyright (c) 2018, Intel Corporation. All rights reserved.
+; This program and the accompanying materials
+; are licensed and made available under the terms and conditions of the BSD
License
+; which accompanies
[edk2] [PATCH v1 0/5] [CVE-2017-5753] Bounds Check Bypass issue in SMI handlers
The series aims to mitigate the Bounds Check Bypass (CVE-2017-5753) issues within SMI handlers. A more detailed explanation of the purpose of the series is under the 'Bounds check bypass mitigation' section of the below link: https://software.intel.com/security-software-guidance/insights/host-firmware-speculative-execution-side-channel-mitigation And the document at: https://software.intel.com/security-software-guidance/api-app/sites/default/files/337879-analyzing-potential-bounds-Check-bypass-vulnerabilities.pdf Cc: Ard Biesheuvel Cc: Laszlo Ersek Cc: Jiewen Yao Cc: Michael D Kinney Cc: Liming Gao Cc: Star Zeng Cc: Eric Dong Hao Wu (5): MdePkg/BaseLib: Add new LoadFence API MdeModulePkg/FaultTolerantWrite:[CVE-2017-5753]Fix bounds check bypass MdeModulePkg/SmmLockBox: [CVE-2017-5753] Fix bounds check bypass MdeModulePkg/Variable: [CVE-2017-5753] Fix bounds check bypass UefiCpuPkg/PiSmmCpuDxeSmm: [CVE-2017-5753] Fix bounds check bypass MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c | 2 ++ MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf | 1 + MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c | 2 ++ MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 1 + MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 3 ++ MdePkg/Include/Library/BaseLib.h | 12 +++ MdePkg/Library/BaseLib/Arm/LoadFence.c | 26 ++ MdePkg/Library/BaseLib/BaseLib.inf | 4 +++ MdePkg/Library/BaseLib/Ebc/CpuBreakpoint.c | 15 +++- MdePkg/Library/BaseLib/Ia32/LoadFence.nasm | 37 +++ MdePkg/Library/BaseLib/X64/LoadFence.nasm | 38 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 1 + 12 files changed, 141 insertions(+), 1 deletion(-) create mode 100644 MdePkg/Library/BaseLib/Arm/LoadFence.c create mode 100644 MdePkg/Library/BaseLib/Ia32/LoadFence.nasm create mode 100644 MdePkg/Library/BaseLib/X64/LoadFence.nasm -- 2.12.0.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v1 4/5] MdeModulePkg/Variable: [CVE-2017-5753] Fix bounds check bypass
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1194
Speculative execution is used by processor to avoid having to wait for
data to arrive from memory, or for previous operations to finish, the
processor may speculate as to what will be executed.
If the speculation is incorrect, the speculatively executed instructions
might leave hints such as which memory locations have been brought into
cache. Malicious actors can use the bounds check bypass method (code
gadgets with controlled external inputs) to infer data values that have
been used in speculative operations to reveal secrets which should not
otherwise be accessed.
This commit will focus on the SMI handler(s) registered within the
Variable\RuntimeDxe driver and insert LoadFence API to mitigate the
bounds check bypass issue.
For SMI handler SmmVariableHandler():
Under "case SMM_VARIABLE_FUNCTION_GET_VARIABLE:",
'SmmVariableHeader->NameSize' can be a potential cross boundary access of
the 'CommBuffer' (controlled external input) during speculative execution.
This cross boundary access is later used as the index to access array
'SmmVariableHeader->Name' by code:
"SmmVariableHeader->Name[SmmVariableHeader->NameSize/sizeof (CHAR16) - 1]"
One can observe which part of the content within array was brought into
cache to possibly reveal the value of 'SmmVariableHeader->NameSize'.
Hence, this commit adds a LoadFence after the boundary/range checks of
'CommBuffer' to prevent the speculative execution.
And there are 2 similar cases under
"case SMM_VARIABLE_FUNCTION_SET_VARIABLE:" and
"case SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET:" as well.
This commits also handles them.
Also, under "case SMM_VARIABLE_FUNCTION_SET_VARIABLE:",
'(UINT8 *)SmmVariableHeader->Name + SmmVariableHeader->NameSize' points to
the 'CommBuffer' (with some offset) and then passed as parameter 'Data' to
function VariableServiceSetVariable().
Within function VariableServiceSetVariable(), there is a sanity check for
EFI_VARIABLE_AUTHENTICATION_2 descriptor for the data pointed by 'Data'.
If this check is speculatively bypassed, potential cross-boundary data
access for 'Data' is possible to be revealed via the below function calls
sequence during speculative execution:
AuthVariableLibProcessVariable()
ProcessVarWithPk() or ProcessVarWithKek()
Within function ProcessVarWithPk() or ProcessVarWithKek(), for the code
"PayloadSize = DataSize - AUTHINFO2_SIZE (Data);", 'AUTHINFO2_SIZE (Data)'
can be a cross boundary access during speculative execution.
Then, 'PayloadSize' is possible to be revealed by the function call
sequence:
AuthServiceInternalUpdateVariableWithTimeStamp()
mAuthVarLibContextIn->UpdateVariable()
VariableExLibUpdateVariable()
UpdateVariable()
CopyMem()
Hence, this commit adds a LoadFence after the sanity check for
EFI_VARIABLE_AUTHENTICATION_2 descriptor upon 'Data' within function
VariableServiceSetVariable() to prevent the speculative execution.
A more detailed explanation of the purpose of commit is under the
'Bounds check bypass mitigation' section of the below link:
https://software.intel.com/security-software-guidance/insights/host-firmware-speculative-execution-side-channel-mitigation
And the document at:
https://software.intel.com/security-software-guidance/api-app/sites/default/files/337879-analyzing-potential-bounds-Check-bypass-vulnerabilities.pdf
Cc: Jiewen Yao
Cc: Star Zeng
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu
---
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c| 1 +
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 3 +++
2 files changed, 4 insertions(+)
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index 1ea2f84dda..52af56c4c0 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -3198,6 +3198,7 @@ VariableServiceSetVariable (
((EFI_VARIABLE_AUTHENTICATION_2 *) Data)->AuthInfo.Hdr.dwLength <
OFFSET_OF (WIN_CERTIFICATE_UEFI_GUID, CertData)) {
return EFI_SECURITY_VIOLATION;
}
+LoadFence ();
PayloadSize = DataSize - AUTHINFO2_SIZE (Data);
} else {
PayloadSize = DataSize;
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
index e495d971a0..0bbed71a76 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
@@ -537,6 +537,7 @@ SmmVariableHandler (
goto EXIT;
}
+ LoadFence ();
if (SmmVariableHeader->NameSize < sizeof (CHAR16) ||
SmmVariableHeader->Name[SmmVariableHeader->NameSize/sizeof (CHAR16) - 1] !=
L'\0') {
//
// Make sure VariableName is A Null-terminated string.
@@ -631,6 +632,7 @@ SmmVariableHandler (
goto EXIT;
}
+ LoadFence ();
if (SmmVariableHeader-
[edk2] [PATCH v1 5/5] UefiCpuPkg/PiSmmCpuDxeSmm: [CVE-2017-5753] Fix bounds check bypass
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1194
Speculative execution is used by processor to avoid having to wait for
data to arrive from memory, or for previous operations to finish, the
processor may speculate as to what will be executed.
If the speculation is incorrect, the speculatively executed instructions
might leave hints such as which memory locations have been brought into
cache. Malicious actors can use the bounds check bypass method (code
gadgets with controlled external inputs) to infer data values that have
been used in speculative operations to reveal secrets which should not
otherwise be accessed.
It is possible for SMI handler(s) to call EFI_SMM_CPU_PROTOCOL service
ReadSaveState() and use the content in the 'CommBuffer' (controlled
external inputs) as the 'CpuIndex'. So this commit will insert LoadFence
API to mitigate the bounds check bypass issue within SmmReadSaveState().
For SmmReadSaveState():
The 'CpuIndex' will be passed into function ReadSaveStateRegister(). And
then in to ReadSaveStateRegisterByIndex().
With the call:
ReadSaveStateRegisterByIndex (
CpuIndex,
SMM_SAVE_STATE_REGISTER_IOMISC_INDEX,
sizeof(IoMisc.Uint32),
&IoMisc.Uint32
);
The 'IoMisc' can be a cross boundary access during speculative execution.
Later, 'IoMisc' is used as the index to access buffers 'mSmmCpuIoWidth'
and 'mSmmCpuIoType'. One can observe which part of the content within
those buffers was brought into cache to possibly reveal the value of
'IoMisc'.
Hence, this commit adds a LoadFence after the check of 'CpuIndex' within
function SmmReadSaveState() to prevent the speculative execution.
A more detailed explanation of the purpose of commit is under the
'Bounds check bypass mitigation' section of the below link:
https://software.intel.com/security-software-guidance/insights/host-firmware-speculative-execution-side-channel-mitigation
And the document at:
https://software.intel.com/security-software-guidance/api-app/sites/default/files/337879-analyzing-potential-bounds-Check-bypass-vulnerabilities.pdf
Cc: Laszlo Ersek
Cc: Jiewen Yao
Cc: Michael D Kinney
Cc: Eric Dong
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu
---
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index fbf74e8d90..256a8bbb94 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -237,6 +237,7 @@ SmmReadSaveState (
if ((CpuIndex >= gSmst->NumberOfCpus) || (Buffer == NULL)) {
return EFI_INVALID_PARAMETER;
}
+ LoadFence ();
//
// Check for special EFI_SMM_SAVE_STATE_REGISTER_PROCESSOR_ID
--
2.12.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v1 2/5] MdeModulePkg/FaultTolerantWrite:[CVE-2017-5753]Fix bounds check bypass
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1194
Speculative execution is used by processor to avoid having to wait for
data to arrive from memory, or for previous operations to finish, the
processor may speculate as to what will be executed.
If the speculation is incorrect, the speculatively executed instructions
might leave hints such as which memory locations have been brought into
cache. Malicious actors can use the bounds check bypass method (code
gadgets with controlled external inputs) to infer data values that have
been used in speculative operations to reveal secrets which should not
otherwise be accessed.
This commit will focus on the SMI handler(s) registered within the
FaultTolerantWriteDxe driver and insert LoadFence API to mitigate the
bounds check bypass issue.
For SMI handler SmmFaultTolerantWriteHandler():
Under "case FTW_FUNCTION_WRITE:", 'SmmFtwWriteHeader->Length' can be a
potential cross boundary access of the 'CommBuffer' (controlled external
inputs) during speculative execution. This cross boundary access is later
passed as parameter 'Length' into function FtwWrite().
Within function FtwWrite(), the value of 'Length' can be inferred by code:
"CopyMem (MyBuffer + Offset, Buffer, Length);". One can observe which part
of the content within 'Buffer' was brought into cache to possibly reveal
the value of 'Length'.
Hence, this commit adds a LoadFence after the boundary/range checks of
'CommBuffer' to prevent the speculative execution.
A more detailed explanation of the purpose of commit is under the
'Bounds check bypass mitigation' section of the below link:
https://software.intel.com/security-software-guidance/insights/host-firmware-speculative-execution-side-channel-mitigation
And the document at:
https://software.intel.com/security-software-guidance/api-app/sites/default/files/337879-analyzing-potential-bounds-Check-bypass-vulnerabilities.pdf
Cc: Jiewen Yao
Cc: Star Zeng
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu
---
MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c | 2 ++
MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf | 1 +
2 files changed, 3 insertions(+)
diff --git
a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c
b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c
index 632313f076..2ed1bb9498 100644
--- a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c
+++ b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c
@@ -57,6 +57,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
EXPRESS OR IMPLIED.
#include
#include
#include
+#include
#include
#include "FaultTolerantWrite.h"
#include "FaultTolerantWriteSmmCommon.h"
@@ -417,6 +418,7 @@ SmmFaultTolerantWriteHandler (
&SmmFvbHandle
);
if (!EFI_ERROR (Status)) {
+LoadFence ();
Status = FtwWrite(
&mFtwDevice->FtwInstance,
SmmFtwWriteHeader->Lba,
diff --git
a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf
b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf
index 85d109e8d9..606cc2266b 100644
--- a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf
+++ b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf
@@ -55,6 +55,7 @@
PcdLib
ReportStatusCodeLib
SmmMemLib
+ BaseLib
[Guids]
#
--
2.12.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v1 3/5] MdeModulePkg/SmmLockBox: [CVE-2017-5753] Fix bounds check bypass
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1194 Speculative execution is used by processor to avoid having to wait for data to arrive from memory, or for previous operations to finish, the processor may speculate as to what will be executed. If the speculation is incorrect, the speculatively executed instructions might leave hints such as which memory locations have been brought into cache. Malicious actors can use the bounds check bypass method (code gadgets with controlled external inputs) to infer data values that have been used in speculative operations to reveal secrets which should not otherwise be accessed. This commit will focus on the SMI handler(s) registered within the SmmLockBox driver and insert LoadFence API to mitigate the bounds check bypass issue. For SMI handler SmmLockBoxHandler(): Under "case EFI_SMM_LOCK_BOX_COMMAND_SAVE:", the 'CommBuffer' (controlled external inputs) is passed to function SmmLockBoxSave(). 'TempLockBoxParameterSave.Length' can be a potential cross boundary access of the 'CommBuffer' during speculative execution. This cross boundary access is later passed as parameter 'Length' into function SaveLockBox(). Within function SaveLockBox(), the value of 'Length' can be inferred by code: "CopyMem ((VOID *)(UINTN)SmramBuffer, (VOID *)(UINTN)Buffer, Length);". One can observe which part of the content within 'Buffer' was brought into cache to possibly reveal the value of 'Length'. Hence, this commit adds a LoadFence after the boundary/range checks of 'CommBuffer' to prevent the speculative execution. And there is a similar case under "case EFI_SMM_LOCK_BOX_COMMAND_UPDATE:" function SmmLockBoxUpdate() as well. This commits also handles it. A more detailed explanation of the purpose of commit is under the 'Bounds check bypass mitigation' section of the below link: https://software.intel.com/security-software-guidance/insights/host-firmware-speculative-execution-side-channel-mitigation And the document at: https://software.intel.com/security-software-guidance/api-app/sites/default/files/337879-analyzing-potential-bounds-Check-bypass-vulnerabilities.pdf Cc: Jiewen Yao Cc: Star Zeng Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Hao Wu --- MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c b/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c index 5a11743cb9..87b4947908 100644 --- a/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c +++ b/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c @@ -76,6 +76,7 @@ SmmLockBoxSave ( LockBoxParameterSave->Header.ReturnStatus = (UINT64)EFI_ACCESS_DENIED; return ; } + LoadFence (); // // Save data @@ -160,6 +161,7 @@ SmmLockBoxUpdate ( LockBoxParameterUpdate->Header.ReturnStatus = (UINT64)EFI_ACCESS_DENIED; return ; } + LoadFence (); // // Update data -- 2.12.0.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch] SecurityPkg/TcgStorageOpalLib: Fixed correct user password not works issue.
Reviewed-by: Hao Wu
Best Regards,
Hao Wu
> -Original Message-
> From: Dong, Eric
> Sent: Monday, September 17, 2018 11:11 AM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A; Chu, Maggie
> Subject: [Patch] SecurityPkg/TcgStorageOpalLib: Fixed correct user password
> not works issue.
>
> After admin password reach the TryLimit value, code logic will direct return
> error password result no matter which password been inputted. So even correct
> user password will return TryLimit error.
>
> Now update code logic to also check user password. Only when both
> user/admin
> password reach the TryLimit count, code will return exceed TryLimit error.
>
> Cc: Wu Hao
> Cc: Chu Maggie
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong
> ---
> SecurityPkg/Library/TcgStorageOpalLib/TcgStorageOpalUtil.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/SecurityPkg/Library/TcgStorageOpalLib/TcgStorageOpalUtil.c
> b/SecurityPkg/Library/TcgStorageOpalLib/TcgStorageOpalUtil.c
> index 3c4a8e9001..f2febc0a0c 100644
> --- a/SecurityPkg/Library/TcgStorageOpalLib/TcgStorageOpalUtil.c
> +++ b/SecurityPkg/Library/TcgStorageOpalLib/TcgStorageOpalUtil.c
> @@ -763,7 +763,6 @@ OpalUtilUpdateGlobalLockingRange(
>
>if (MethodStatus ==
> TCG_METHOD_STATUS_CODE_AUTHORITY_LOCKED_OUT) {
> DEBUG ((DEBUG_INFO, "unlock as admin failed with
> AUTHORITY_LOCKED_OUT\n"));
> -goto done;
>}
>
>//
> --
> 2.15.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
