Re: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

["Yao, Jiewen" <jiewen.yao@intel.com>]

Thank you Laszlo.

Eric and I communicated with internal team. They understand that CPU is the only owner of the CPU state (CR3/DGT/IDT) and MP_SERVICE_PROTOCOL. Then we agree that the original test is invalid.

As conclusion, we can withdraw this patch and close the Bugzilla with "not a defect". 😊

Thank you
Yao Jiewen

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Friday, September 4, 2020 4:37 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
> vanjeff_919@hotmail.com; Dong, Eric <eric.dong@intel.com>; Ni, Ray
> <ray.ni@intel.com>
> Cc: Lou, Yun <yun.lou@intel.com>
> Subject: Re: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for
> CR3/GDT/IDT.
> 
> On 09/04/20 10:06, Yao, Jiewen wrote:
> > When we say “validate”, we need get clear on what is the contract between
> caller and callee, and what is the expectation of the validation.
> >
> > For example, in this case, the validation is limited to 4G or not 4G. Why?
> > This function does not verify following, why?
> >
> >   1.  if the GDT table is valid.
> >   2.  if the IDT table is valid
> >   3.  if the exception handler is valid
> >   4.  if the timer handler still works
> >   5.  if the page table is valid
> >   6.  if the page table is 1:1 mapping
> >   7.  if the page table still enforce the expected protection, such as RO, NX
> >   8.  ……
> 
> Yes, very good point; it has crossed my mind before too. The currently
> proposed checks verify the CR3 (but not the end of the root page
> directory). They also don't try to walk the whole forest of page tables
> and check every entry against 4GB (or, as you say, for 1:1 mapping). The
> check covers the GDT and the IDT, but not the GDT and IDT entries
> (segment granularity? direction of growth?)
> 
> I'm OK with the proposed rudimentary checks because in my mind they are
> supposed to catch only one idiosyncratic UEFI application.
> 
> > If an application creates a crazy state, CpuDxe may pass the validation with
> below 4G page table, but still fail to wake up APs.
> 
> Yes, absolutely.
> 
> >
> > If it is the app’s responsibility to ensure the system in good state, the
> validation is unnecessary.
> > If it is the CpuDxe’s responsibility to ensure the system in good state, the
> validation is insufficient.
> 
> Agreed on both counts.
> 
> I'm just under the impression that Eric has some internal use case that
> doesn't let him fix the application -- or maybe there's no time left for
> them for fixing the application.
> 
> >
> > I think we should wait. I am working with Eric to collect the requirement on
> why test case is designed in this way.
> 
> Sounds good, thanks!
> 
> >
> > DebugAgentDxe is a good case. It is for debug purpose.
> > I believe there must be contract between CPU driver and DebugAgentDxe that
> which status DebugAgentDxe may modify and which state DebugAgentDxe may
> not.
> > It is DebugAgentDxe’s responsibility to ensure the new system state is correct
> and compatible with the CPU driver.
> 
> Agreed 100%
> 
> > With the contract, I don’t think CPU driver need validate the system state
> changed by DebugAgentDxe.
> > If DebugAgentDxe put system in a wrong state, CPUDriver has no chance to
> run.
> 
> Agreed again.
> 
> Thanks
> Laszlo
> 
> 
>