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>
Sent: Friday, April 18, 2025 11:26 AM
To: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>
Cc: devel@edk2.groups.io; Ni, Ray <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>
Sent: Friday, April 18, 2025 8:59 AM
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 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>
Sent: Friday, April 18, 2025 12:30 AM
To: Ni, Ray <ray.ni@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>;
devel@edk2.groups.io; Ni, Ray <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>
Sent: Thursday, April 17, 2025 11:45 PM
To: Kun Qin <Kun.Qin@microsoft.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>;
devel@edk2.groups.io; Ni, Ray <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