Re: [edk2-devel] A bug in the SmmCommunication V3 logic

["Kun Qin via groups.io" <Kun.Qin=microsoft.com@groups.io>]

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

Hi Mike,

I created a PR on edk2 site but I was thinking about redoing it... Because of the v1 implementation is a bit chaotic.

The original form are still broken if we back out the change, according to our discussion below. Specifically for v1, we cannot touch the incoming buffer after going virtual at all, because we need the incoming buffer to be physical to populate the gSmmCorePrivate->CommunicationBuffer pointer. Otherwise, the validation inside SMM will work pass.

On the StMm side, we are partially covered because MM communicate on standalone mm will always intake the incoming and copy it over to the static buffer `CopyMem ((VOID *)(UINTN)mMmCommonBuffer.PhysicalStart, CommBuffer, BufferSize);`. However, this bears the assumption that we will need to pass in virtual address when invoking Stmm backed MM communicate v1 to make it work, which is a contradictory expectation compared to traditional MM communicate protocol.

One solution is to make comm size a requirement for v1, and v2/3 can check the incoming content through the virtual address. Please let me know your thoughts.

Thanks,
Kun

From: Kinney, Michael D <michael.d.kinney@intel.com>
Sent: Friday, April 18, 2025 2:34 PM
To: Kun Qin <Kun.Qin@microsoft.com>; Ni, Ray <ray.ni@intel.com>
Cc: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: [EXTERNAL] RE: A bug in the SmmCommunication V3 logic

Hi Kun,

I have a proposed solution that puts SmmCommunicationCommunicate() back to its original form, so there are no changes in behavior for SmmCommunicationCommunicate() or SmmCommunicationMmCommunicate2().

I then pull the V3 only logic into MmCommunicationMmCommunicate3().

It has some code duplication, but makes it easier to understand and uses the virtual and physical pointers in the right places for the V3 implementation.

What were you thinking?

Mike


From: Kun Qin <Kun.Qin@microsoft.com<mailto:Kun.Qin@microsoft.com>>
Sent: Friday, April 18, 2025 11:26 AM
To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
Subject: RE: A bug in the SmmCommunication V3 logic

Hi Mike,

Thanks for the explanation. That makes sense. I confirmed the issue on PiSmmIpl side. A patch should be available some time today after I test both traditional MM and Standalone MM paths.

Regards,
Kun

From: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Sent: Friday, April 18, 2025 8:59 AM
To: Kun Qin <Kun.Qin@microsoft.com<mailto:Kun.Qin@microsoft.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Subject: [EXTERNAL] RE: A bug in the SmmCommunication V3 logic

Hi Kun,

I think the reason it may have always worked is for the case where SmmCommunicationCommunicate() at runtime after SetVirtualAddressMap() and CommSize is not NULL.  In that case, CommunicateHeader that points to CommBuffer is never dereferenced and a call to mSmmControl2->Trigger() is made.

If CommSize is NULL, then MessageLength field of CommunicateHeader would be dereferenced as a physical address at OS runtime after SetVirtualAddressMap() and a page fault would occur.  My guess is that CommSize is never NULL, and that is why we have been lucky.

With the addition of the check for the V3 GUID, the HeaderGuid field of CommunicateHeader is always dereferenced.  So if CommunicateHeader is a physical address at OS Runtime after SetVirtualAddressMap(), then a page fault will always occur.  No more luck.

Mike

From: Kun Qin <Kun.Qin@microsoft.com<mailto:Kun.Qin@microsoft.com>>
Sent: Friday, April 18, 2025 12:30 AM
To: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
Subject: RE: A bug in the SmmCommunication V3 logic

Hi Ray,

I will verify the code first thing tomorrow morning. But I just looked at the code flow before the change, it seems that SmmCommunicationMmCommunicate2 is also using physical address, and the common routine will deference the pointer to read message length as well. I just checked variable runtime driver did not convert the input physical pointer. Would that cause the same issue? Did I miss something or we have lucked out all along?

Regards,
Kun

From: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
Sent: Thursday, April 17, 2025 11:45 PM
To: Kun Qin <Kun.Qin@microsoft.com<mailto:Kun.Qin@microsoft.com>>
Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
Subject: [EXTERNAL] A bug in the SmmCommunication V3 logic

Hi Qin,
I think there is a bug in the SmmCommunication protocol implementation.

All 3 communication protocol calls go to the same communicate() function that tests the HeaderGuid against the V3 GUID.
But when the call is from runtime, reading the HeaderGuid using the physical address of communication buffer would cause page fault. The virtual address should be used.
The bug was not there without your patch because the communicate routines happened not to read any bytes from the communication buffer but simply pass the address to SMM. SMM expects the physical address because the virtual-to-physical mapping in SMM is identical.

The bug exists in both the SmmIpl.c in MdeModulePkg and the MmCommunicationDxe.c in StandaloneMmPkg.
The bug would cause OS boot failure if there is any communication protocol invocation after ExitBootService.

I guess the bug might not be there in your first version of patch, but was introduced when I asked you to consolidate the logic together.

Can you kindly reproduce it locally and send out a fix after confirming?

Thanks,
Ray


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121271): https://edk2.groups.io/g/devel/message/121271
Mute This Topic: https://groups.io/mt/112327494/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



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