On 07/02/20 07:15, Guomin Jiang wrote:
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1614
>
> To avoid the TOCTOU, enable paging and set Not Present flag so when
> access any code in the flash range, it will trigger #NP exception.
>
> Cc: Eric Dong
> Cc: Ray Ni
> Cc: Laszlo Ersek
> Cc: Rahul Kumar
> Signed-off-by: Guomin Jiang
> ---
> UefiCpuPkg/CpuMpPei/CpuMpPei.inf | 3 +++
> UefiCpuPkg/CpuMpPei/CpuPaging.c | 17 +++--
> 2 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
> b/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
> index caead3ce34d4..fd50b55f06cb 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
> +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
> @@ -46,6 +46,9 @@ [LibraryClasses]
>BaseMemoryLib
>CpuLib
>
> +[Guids]
> + gEdkiiMigratedFvInfoGuid ##
> SOMETIMES_CONSUMES ## HOB
> +
> [Ppis]
>gEfiPeiMpServicesPpiGuid ## PRODUCES
>gEfiSecPlatformInformationPpiGuid ## SOMETIMES_CONSUMES
> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> index d0cbebf70bbf..af4069b42cdb 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> @@ -12,6 +12,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include
> #include
> #include
> +#include
>
> #include "CpuMpPei.h"
>
> @@ -605,6 +606,8 @@ MemoryDiscoveredPpiNotifyCallback (
>EFI_STATUS Status;
>BOOLEAN InitStackGuard;
>BOOLEAN InterruptState;
> + EDKII_MIGRATED_FV_INFO *MigratedFvInfo;
> + EFI_PEI_HOB_POINTERS Hob;
>
>InterruptState = SaveAndDisableInterrupts ();
>Status = MigrateGdt ();
> @@ -617,9 +620,9 @@ MemoryDiscoveredPpiNotifyCallback (
>// the task switch (for the sake of stack switch).
>//
>InitStackGuard = FALSE;
> - if (IsIa32PaeSupported () && PcdGetBool (PcdCpuStackGuard)) {
> + if (IsIa32PaeSupported ()) {
> EnablePaging ();
> -InitStackGuard = TRUE;
> +InitStackGuard = PcdGetBool (PcdCpuStackGuard);
>}
>
>Status = InitializeCpuMpWorker ((CONST EFI_PEI_SERVICES **)PeiServices);
> @@ -629,6 +632,16 @@ MemoryDiscoveredPpiNotifyCallback (
> SetupStackGuardPage ();
>}
>
> + Hob.Raw = GetFirstGuidHob ();
> + while (Hob.Raw != NULL) {
> +MigratedFvInfo = GET_GUID_HOB_DATA (Hob);
> +ConvertMemoryPageAttributes (MigratedFvInfo->FvOrgBase,
> MigratedFvInfo->FvLength, 0);
> +
> +Hob.Raw = GET_NEXT_HOB (Hob);
> +Hob.Raw = GetNextGuidHob (, Hob.Raw);
> + }
> + CpuFlushTlb ();
> +
>return Status;
> }
>
>
(1) This patch calls EnablePaging() even if no
"gEdkiiMigratedFvInfoGuid" HOB exists.
In other words, assume "PcdMigrateTemporaryRamFirmwareVolumes" is FALSE.
- In that case, PeiCore() will not call EvacuateTempRam().
- Consequently, zero "gEdkiiMigratedFvInfoGuid" HOBs are going to be built.
- Consequently, this patch will never call ConvertMemoryPageAttributes()
Why do we call EnablePaging() then (assuming IsIa32PaeSupported()
returns TRUE)?
(2) Consider the opposite case. Assume IsIa32PaeSupported() returns
FALSE. Further assume that at least one "gEdkiiMigratedFvInfoGuid" HOB
exists.
Then ConvertMemoryPageAttributes() will be called, without us ever
calling EnablePaging(). That doesn't seem right.
I suggest:
BOOLEAN InitStackGuard;
EFI_PEI_HOB_POINTERS Hob;
InitStackGuard = FALSE;
Hob.Raw = NULL;
//
// Both the "stack guard" feature and the "temp RAM evacuation"
// feature depend on IA32 PAE support.
//
if (IsIa32PaeSupported ()) {
InitStackGuard = PcdGetBool (PcdCpuStackGuard);
Hob.Raw = GetFirstGuidHob ();
}
//
// If either feature is active, then paging is required; enable it.
//
if (InitStackGuard || Hob.Raw != NULL) {
EnablePaging ();
}
/* ... */
if (InitStackGuard) {
SetupStackGuardPage ();
}
/* note: no need to call GetFirstGuidHob() again! */
while (Hob.Raw != NULL) {
/* ... */
}
CpuFlushTlb ();
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#62031): https://edk2.groups.io/g/devel/message/62031
Mute This Topic: https://groups.io/mt/75252669/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
