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

Fan Jeff vanjeff_919 at hotmail.com
Fri Sep 4 08:43:41 UTC 2020 

Jiewen,

I don’t think AP need to validate everyting. BSP should make sure CR3/GDT/IDT corrently firstly.  Othersize, everything does not make sense.

For CR3 case, if we assume CR3 above 4GB space is legal for BSP (If there are any limitation on it, please corrent me) but the current CPU AP wakeup method does not allow CR3 above 4GB space, this maybe the CPU driver’s bug or implementation limitation.

The key is that CR3 above 4GB is legal or not.
If we think this is bug, we need to enhance CPU driver to fix it.
If we think this is one limiation, we need to validate the CR3 value and tell the caller.

Jeff

发件人: Yao, Jiewen<mailto:jiewen.yao at intel.com>
发送时间: 2020年9月4日 16:07
收件人: devel at edk2.groups.io<mailto:devel at edk2.groups.io>; vanjeff_919 at hotmail.com<mailto:vanjeff_919 at hotmail.com>; Laszlo Ersek<mailto:lersek at redhat.com>; Dong, Eric<mailto:eric.dong at intel.com>; Ni, Ray<mailto:ray.ni at intel.com>
抄送: Lou, Yun<mailto:yun.lou at intel.com>
主题: RE: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

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.  ……

If an application creates a crazy state, CpuDxe may pass the validation with below 4G page table, but still fail to wake up APs.

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.

I think we should wait. I am working with Eric to collect the requirement on why test case is designed in this way.

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.
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.




From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Fan Jeff
Sent: Friday, September 4, 2020 3:32 PM
To: Laszlo Ersek <lersek at redhat.com>; devel at edk2.groups.io; Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at intel.com>
Cc: Lou, Yun <yun.lou at intel.com>
Subject: 回复: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

Laszlo,

Why sync the BSP’s CR3/GDT/IDT values for AP when AP wakes up instead of using old cached BSP’s CR3/GDT/IDT values when CPU driver initiallly dispatched is that we CANNOT assume original values are still valid during POST phase.

On this senario, BSP’s CR3/GDT/IDT are just like input parameters for one function. Validating them are necessary.

For example, DebugAgentDxe driver may be loaded in UEFI shell to setup source level debugging enviroment of EDKII debugger tools.  It may setup new IDT space.

Jeff

发件人: Laszlo Ersek<mailto:lersek at redhat.com>
发送时间: 2020年9月4日 14:58
收件人: Fan Jeff<mailto:vanjeff_919 at hotmail.com>; devel at edk2.groups.io<mailto:devel at edk2.groups.io>; eric.dong at intel.com<mailto:eric.dong at intel.com>; Ni, Ray<mailto:ray.ni at intel.com>
抄送: Lou, Yun<mailto:yun.lou at intel.com>
主题: Re: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

On 09/04/20 04:18, Fan Jeff wrote:
> Laszlo & Eric,
>
> Introducing one new PCD to handle such rare test case is too heavy. I think We could do validating CR3/GDT/IDT space < 4GB address always in MpInitLib.

I disagree.

What the UEFI application does (interfere with GDT / IDT / CR3
placement) is invalid. It changes system properties under the feet of
platform DXE drivers. UEFI applications are supposed to be written
against public protocols and services in the UEFI spec (and maybe in the
PI spec).

If this application is a test application that purposely massages
low-level system properties, that's fine; but then, if we change core
edk2 components to be somewhat compatible with this application, then we
should make sure that platforms that do not care about this specific use
case do not suffer a performance hit or a code complexity hit.

What I could accept, under your proposal, is the following: add three
ASSERT()s to FillExchangeInfoData(), where we fetch the IDTR / GDTR /
CR3 anyway. This would be fine because it only expresses existing
assumptions / requirements.

However, my understanding is that this would not solve Eric's problem.
The system would hang -- in DEBUG / NOOPT builds -- or crash -- in a
RELEASE build -- just the same as before.

Now, *if* FillExchangeInfoData() is currently *wrong* to have these
32-bit assumptions, because edk2 modules themselves can break those
assumptions (without the custom UEFI application), then we have a more
serious problem. But for that problem, just "checking and rejecting" is
not a sufficient solution, regardless of how and where we check and reject.

Thanks
Laszlo




>
> Jeff
>
> 发件人: Dong, Eric<mailto:eric.dong at intel.com>
> 发送时间: 2020年9月4日 10:01
> 收件人: Ni, Ray<mailto:ray.ni at intel.com>; devel at edk2.groups.io<mailto:devel at edk2.groups.io<mailto:devel at edk2.groups.io%3cmailto:devel at edk2.groups.io>>; lersek at redhat.com<mailto:lersek at redhat.com>
> 抄送: Lou, Yun<mailto:yun.lou at intel.com>
> 主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
>
> I guess Laszlo think this check is not always needed, just used for this special shell application case. He wants to use default FALSE to always ignore this check and make code logic clear.
>
> Thanks,
> Eric
>
> From: Ni, Ray <ray.ni at intel.com<mailto:ray.ni at intel.com>>
> Sent: Friday, September 4, 2020 9:34 AM
> To: devel at edk2.groups.io<mailto:devel at edk2.groups.io>; lersek at redhat.com<mailto:lersek at redhat.com>; Dong, Eric <eric.dong at intel.com<mailto:eric.dong at intel.com>>
> Cc: Lou, Yun <yun.lou at intel.com<mailto:yun.lou at intel.com>>
> Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
>
> Why do we need a new PCD to control such check? Under what circumstance the PCD is false?
> We may need to move such check out of MpLib.c. Because when bps runs at 32bit mode, AP doesn’t need to switch to long mode, such check is not needed and also always passes.
>
> We should not assume PEI runs at 32 bit mode.
>
>
> 发件人: devel at edk2.groups.io<mailto:devel at edk2.groups.io<mailto:devel at edk2.groups.io%3cmailto:devel at edk2.groups.io>> <devel at edk2.groups.io<mailto:devel at edk2.groups.io<mailto:devel at edk2.groups.io%3cmailto:devel at edk2.groups.io>>> 代表 Laszlo Ersek <lersek at redhat.com<mailto:lersek at redhat.com<mailto:lersek at redhat.com%3cmailto:lersek at redhat.com>>>
> 发送时间: Friday, September 4, 2020 3:55:47 AM
> 收件人: Dong, Eric <eric.dong at intel.com<mailto:eric.dong at intel.com<mailto:eric.dong at intel.com%3cmailto:eric.dong at intel.com>>>; devel at edk2.groups.io<mailto:devel at edk2.groups.io<mailto:devel at edk2.groups.io%3cmailto:devel at edk2.groups.io>> <devel at edk2.groups.io<mailto:devel at edk2.groups.io<mailto:devel at edk2.groups.io%3cmailto:devel at edk2.groups.io>>>
> 抄送: Ni, Ray <ray.ni at intel.com<mailto:ray.ni at intel.com<mailto:ray.ni at intel.com%3cmailto:ray.ni at intel.com>>>
> 主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
>
> On 09/03/20 21:00, Laszlo Ersek wrote:
>
>> (10) More importantly, ValidCR3GdtIdtCheck() should not be called in the
>> Worker functions for StartupAllAPs, StartupThisAP, SwitchBSP, and
>> EnableDisableAP, in "UefiCpuPkg/Library/MpInitLib/MpLib.c".
>>
>> Instead, the calls should be made in the DXE instance of the library
>> ("UefiCpuPkg/Library/MpInitLib/DxeMpLib.c"), at the very top of the
>> functions:
>>
>> - MpInitLibStartupAllAPs
>> - MpInitLibStartupThisAP
>> - MpInitLibSwitchBSP
>> - MpInitLibEnableDisableAP
>>
>> Here's why:
>>
>> (a) The symptom does not affect the PEI phase -- by the time the UEFI
>> application is executed, the PEI phase has ended; there's no need to
>> modify the PEI instance of the library.
>>
>> (b) The currently proposed failure exits are too late. For example, in
>> the SwitchBSPWorker() function, by the time we exit, we have called
>> DisableApicTimerInterrupt(), SaveAndDisableInterrupts(), and
>> DisableLvtInterrupts() -- and the error path does not restore the
>> original environment.
>>
>> (c) According to the PI spec (v1.7), the StartupAllAPs(),
>> StartupThisAP(), SwitchBSP(), EnableDisableAP() member functions of
>> EFI_MP_SERVICES_PROTOCOL may only be called on the (current) BSP.
>> Because of this, it is OK to call ValidCR3GdtIdtCheck() as the very
>> first action in the above-listed DxeMpLib functions.
>>
>> (Assuming the protocol members are called from an AP, and consequently
>> we check CR3 / GDTR / IDTR on the AP (and not on the BSP), that's the
>> *caller's* fault, per spec!)
>
> This means we can move the ValidCr3GdtIdtCheck() function to
> "DxeMpLib.c", and it is not necessary to modify "MpLib.h".
>
> Thanks
> Laszlo
> >




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#65047): https://edk2.groups.io/g/devel/message/65047
Mute This Topic: https://groups.io/mt/76625679/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20200904/7334ce90/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 154E6E9E578E47869D1F0A70DF84BCF0.png
Type: image/png
Size: 144 bytes
Desc: 154E6E9E578E47869D1F0A70DF84BCF0.png
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20200904/7334ce90/attachment.png>

More information about the edk2-devel-archive mailing list