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

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

[Ni, Ray via groups.io]

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

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 (#121264): https://edk2.groups.io/g/devel/message/121264
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: 4930 bytes --]

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

[Kun Qin via groups.io]

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

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121266): https://edk2.groups.io/g/devel/message/121266
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: 7155 bytes --]

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

[Michael D Kinney via groups.io]

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

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<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 (#121265): https://edk2.groups.io/g/devel/message/121265
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: 9737 bytes --]

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

[Kun Qin via groups.io]

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

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<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 (#121270): https://edk2.groups.io/g/devel/message/121270
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: 11922 bytes --]

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

[Michael D Kinney via groups.io]

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

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<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 (#121267): https://edk2.groups.io/g/devel/message/121267
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: 14298 bytes --]

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

[Kun Qin via 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 --]

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

[Michael D Kinney via groups.io]

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

I think my proposal is better to have separate logic.  One for V1/V2 and another for V3.

I agree that V1 must pass in non NULL CommSize and we can do spec change for that since NULL CommSize is not implementable.

V2 can support CommSize optional because it has phys and virt.

V3 does not have CommSize.

Mike



From: Kun Qin <Kun.Qin@microsoft.com>
Sent: Friday, April 18, 2025 3:18 PM
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,

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<mailto:michael.d.kinney@intel.com>>
Sent: Friday, April 18, 2025 2:34 PM
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 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 (#121268): https://edk2.groups.io/g/devel/message/121268
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: 23701 bytes --]

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

[Kun Qin via groups.io]

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

Okay, I can change that.

On the other hand, how do we want handle the difference between MM communicate for stmm and traditional MM? stmm expects the incoming to be virtual and will always touch the buffer. But traditional mm lucked out by never touching the incoming.

Regards,
Kun

From: Kinney, Michael D <michael.d.kinney@intel.com>
Sent: Friday, April 18, 2025 3:23 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

I think my proposal is better to have separate logic.  One for V1/V2 and another for V3.

I agree that V1 must pass in non NULL CommSize and we can do spec change for that since NULL CommSize is not implementable.

V2 can support CommSize optional because it has phys and virt.

V3 does not have CommSize.

Mike



From: Kun Qin <Kun.Qin@microsoft.com<mailto:Kun.Qin@microsoft.com>>
Sent: Friday, April 18, 2025 3:18 PM
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,

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<mailto:michael.d.kinney@intel.com>>
Sent: Friday, April 18, 2025 2:34 PM
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 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 (#121272): https://edk2.groups.io/g/devel/message/121272
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: 25957 bytes --]

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

[Michael D Kinney via groups.io]

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

Take a look at this branch with one commit that implements my proposal so far

https://github.com/mdkinney/edk2/commits/MdeModulePkg_Core_PiSmmCore_Virtual_Address_Fix/

What version of MM Communicate is used with STMM?

Mike

From: Kun Qin <Kun.Qin@microsoft.com>
Sent: Friday, April 18, 2025 3:34 PM
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

Okay, I can change that.

On the other hand, how do we want handle the difference between MM communicate for stmm and traditional MM? stmm expects the incoming to be virtual and will always touch the buffer. But traditional mm lucked out by never touching the incoming.

Regards,
Kun

From: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Sent: Friday, April 18, 2025 3:23 PM
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

I think my proposal is better to have separate logic.  One for V1/V2 and another for V3.

I agree that V1 must pass in non NULL CommSize and we can do spec change for that since NULL CommSize is not implementable.

V2 can support CommSize optional because it has phys and virt.

V3 does not have CommSize.

Mike



From: Kun Qin <Kun.Qin@microsoft.com<mailto:Kun.Qin@microsoft.com>>
Sent: Friday, April 18, 2025 3:18 PM
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,

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<mailto:michael.d.kinney@intel.com>>
Sent: Friday, April 18, 2025 2:34 PM
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 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 (#121269): https://edk2.groups.io/g/devel/message/121269
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: 27803 bytes --]

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

[Kun Qin via groups.io]

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

Current MM communicate DXE driver supports all 3 versions. I guess v1 is never invoked in Stmm based solutions. I will update the IPL code for SMM shortly.

Regards,
Kun

From: Kinney, Michael D <michael.d.kinney@intel.com>
Sent: Friday, April 18, 2025 3:37 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

Take a look at this branch with one commit that implements my proposal so far

https://github.com/mdkinney/edk2/commits/MdeModulePkg_Core_PiSmmCore_Virtual_Address_Fix/

What version of MM Communicate is used with STMM?

Mike

From: Kun Qin <Kun.Qin@microsoft.com<mailto:Kun.Qin@microsoft.com>>
Sent: Friday, April 18, 2025 3:34 PM
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

Okay, I can change that.

On the other hand, how do we want handle the difference between MM communicate for stmm and traditional MM? stmm expects the incoming to be virtual and will always touch the buffer. But traditional mm lucked out by never touching the incoming.

Regards,
Kun

From: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Sent: Friday, April 18, 2025 3:23 PM
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

I think my proposal is better to have separate logic.  One for V1/V2 and another for V3.

I agree that V1 must pass in non NULL CommSize and we can do spec change for that since NULL CommSize is not implementable.

V2 can support CommSize optional because it has phys and virt.

V3 does not have CommSize.

Mike



From: Kun Qin <Kun.Qin@microsoft.com<mailto:Kun.Qin@microsoft.com>>
Sent: Friday, April 18, 2025 3:18 PM
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,

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<mailto:michael.d.kinney@intel.com>>
Sent: Friday, April 18, 2025 2:34 PM
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 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 (#121273): https://edk2.groups.io/g/devel/message/121273
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: 29701 bytes --]

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

[Michael D Kinney via groups.io]

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

I would recommend not using V1 and updating callers to use V2 or V3.

We could consider deprecating V1 since it is hard to use and has limitations.

Mike

From: Kun Qin <Kun.Qin@microsoft.com>
Sent: Friday, April 18, 2025 3:46 PM
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

Current MM communicate DXE driver supports all 3 versions. I guess v1 is never invoked in Stmm based solutions. I will update the IPL code for SMM shortly.

Regards,
Kun

From: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Sent: Friday, April 18, 2025 3:37 PM
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

Take a look at this branch with one commit that implements my proposal so far

https://github.com/mdkinney/edk2/commits/MdeModulePkg_Core_PiSmmCore_Virtual_Address_Fix/

What version of MM Communicate is used with STMM?

Mike

From: Kun Qin <Kun.Qin@microsoft.com<mailto:Kun.Qin@microsoft.com>>
Sent: Friday, April 18, 2025 3:34 PM
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

Okay, I can change that.

On the other hand, how do we want handle the difference between MM communicate for stmm and traditional MM? stmm expects the incoming to be virtual and will always touch the buffer. But traditional mm lucked out by never touching the incoming.

Regards,
Kun

From: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Sent: Friday, April 18, 2025 3:23 PM
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

I think my proposal is better to have separate logic.  One for V1/V2 and another for V3.

I agree that V1 must pass in non NULL CommSize and we can do spec change for that since NULL CommSize is not implementable.

V2 can support CommSize optional because it has phys and virt.

V3 does not have CommSize.

Mike



From: Kun Qin <Kun.Qin@microsoft.com<mailto:Kun.Qin@microsoft.com>>
Sent: Friday, April 18, 2025 3:18 PM
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,

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<mailto:michael.d.kinney@intel.com>>
Sent: Friday, April 18, 2025 2:34 PM
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 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 (#121274): https://edk2.groups.io/g/devel/message/121274
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: 31215 bytes --]