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

["Dong, Eric" <eric.dong@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4076 bytes --]

yes, I will close that Bugzilla. Thanks all for your feedback.

Thanks,
Eric

From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Saturday, September 5, 2020 8:31 PM
To: devel@edk2.groups.io; lersek@redhat.com; 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.

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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Laszlo Ersek
> Sent: Friday, September 4, 2020 4:37 PM
> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>;
> vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Ni, Ray
> <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> Cc: Lou, Yun <yun.lou@intel.com<mailto: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
>
>
> 

[-- Attachment #2: Type: text/html, Size: 13307 bytes --]