On 11/23/15 16:32, Yao, Jiewen wrote: > Comments below: > > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Monday, November 23, 2015 10:39 PM > To: Yao, Jiewen; edk2-de...@ml01.01.org > Cc: Kinney, Michael D; Fan, Jeff > Subject: Re: [edk2] [patch] UefiCpuPkg/PiSmmCpu: Move > RestoreSmmConfigurationInS3 from BSPHandler() to PerformRemainingTasks(). > > Hi Jiewen, > > On 11/23/15 14:50, jiewen yao wrote: >> In this way, we can centralize the silicon configuration in >> PerformRemainingTasks() function. >> If there are more features need to be configured, they can put in >> PerformRemainingTasks() only. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Yao, Jiewen <jiewen....@intel.com> >> Cc: Fan, Jeff <jeff....@intel.com> >> Cc: Kinney, Michael D <michael.d.kin...@intel.com> > > Meta comment: Jordan informed us earlier that commas in email addresses like > the above should be protected with quotes: > > Signed-off-by: "Yao, Jiewen" <jiewen....@intel.com> > Cc: "Fan, Jeff" <jeff....@intel.com> > Cc: "Kinney, Michael D" <michael.d.kin...@intel.com> > > [Jiewen] Got it. > > Some other comments / questions below: > >> --- >> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 11 ----------- >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 11 +++++++++++ >> 2 files changed, 11 insertions(+), 11 deletions(-) >> >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c >> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c >> index 730c32d..d9c5ae4 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c >> @@ -412,17 +412,6 @@ BSPHandler ( >> AcquireSpinLockOrFail (&mSmmMpSyncData->CpuData[CpuIndex].Busy); >> >> // >> - // Restore SMM Configuration in S3 boot path. >> - // >> - if (mRestoreSmmConfigurationInS3) { >> - // >> - // Configure SMM Code Access Check feature if available. >> - // >> - ConfigSmmCodeAccessCheck (); >> - mRestoreSmmConfigurationInS3 = FALSE; >> - } >> - >> - // >> // Invoke SMM Foundation EntryPoint with the processor information >> context. >> // >> gSmmCpuPrivate->SmmCoreEntry >> (&gSmmCpuPrivate->SmmCoreEntryContext); >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> index de681c0..e5cbdbd 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> @@ -1443,4 +1443,15 @@ PerformRemainingTasks ( >> // >> mSmmReadyToLock = FALSE; >> } >> + >> + // >> + // Restore SMM Configuration in S3 boot path. >> + // >> + if (mRestoreSmmConfigurationInS3) { >> + // >> + // Configure SMM Code Access Check feature if available. >> + // >> + ConfigSmmCodeAccessCheck (); >> + mRestoreSmmConfigurationInS3 = FALSE; } >> } >> > > (1) What does ConfigSmmCodeAccessCheck() do? I don't know what "SMM Code > Access Check feature" means. Is it a security feature that enforces that in > SMM code only from SMRAM can be executed? > [Jiewen] Yes, you are right. > > (2) Is it safe to delay that until after SmmCoreEntry (== > SmmEntryPoint()) returns? Just because it looks like that at that point the > SMI processing will be mostly complete. > > I.e., you might already be past most of the "attack surface". It > *vaguely* appears "cart before the horse" to me, but I don't know enough to > state that authoritatively :) > > [Jiewen] Very good question. Current S3Resume driver will always trigger a > dummy SMI, just after relocation. The purpose is to set SMRR, or perform any > lock if required. > This action is always done in PEI Phase, before BootScript running, no any > 3rd part code running, which means we are still in trusted region at that > point. So I think it should be safe. > SMM Code Access Check feature is to help us find out if there is anything > wrong. It is policy-enforcement. The exception should never be triggered in > normal case. > Also, we can use page table to do such policy-enforcement as well, which will > take effect during before SmiRendezvous(). Then we do not need worry about it > at all.
Okay, thanks. > (3) In the PerformRemainingTasks() function, there is already a call > to ConfigSmmCodeAccessCheck(); it depends on "mSmmReadyToLock". After > some staring at the code, I *think* that those two branches will be > exclusive (because only SmmReadyToLockEventNotify() sets > "mSmmReadyToLock", which runs only during normal boot). > > But, I think that a short comment to this effect would be helpful, in > PerformRemainingTasks(). > > [Jiewen] You statement are correct. > I am sorry. Would you please enlighten me on what comment to add? I do not > get it. I was just surprised to see that going forward there would be two calls to ConfigSmmCodeAccessCheck() in PerformRemainingTasks(). After looking more closely, I thought it was justified, but I also thought it prudent to spell out that *at most one* of those branches could be executed in any single invocation of the function. Thus, something like (just the idea, not necessarily the exact wording): // // BSPHandler() calls PerformRemainingTasks() in three mutually // exclusive contexts: // - (mSmmReadyToLock == TRUE): on the normal boot path, when SMM is // being locked down; // - (mRestoreSmmConfigurationInS3 == TRUE): on the S3 resume path; // - otherwise: at boot services time or at runtime, without special // circumstances. // ASSERT (!mSmmReadyToLock || !mRestoreSmmConfigurationInS3); Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
