Skip to content

[Rebase & FF] Adding MM communication buffer validation - #1846

Open
kuqin12 wants to merge 5 commits into
microsoft:release/202511from
kuqin12:mm_comm
Open

[Rebase & FF] Adding MM communication buffer validation#1846
kuqin12 wants to merge 5 commits into
microsoft:release/202511from
kuqin12:mm_comm

Conversation

@kuqin12

@kuqin12 kuqin12 commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Description

This change adds a few validation before using the derived buffer size from message length.

However, note that the buffer size is not used directly due to a check after that. Thus the MM communication agent or the core in MM will not consume the value directly. But this would cause the MM handler to potentially receive a huge message in length.

  • Impacts functionality?
  • Impacts security?
  • Breaking change?
  • Includes tests?
  • Includes documentation?

How This Was Tested

This is tested on QEMU ARM Virt platform and booted to Windows desktop.

Integration Instructions

N/A

@kuqin12
kuqin12 marked this pull request as draft July 10, 2026 23:47
@mu-automation

mu-automation Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

❌ QEMU Validation Failed

Source Dependencies

Repository Commit
mu_basecore 48dff23
mu_tiano_platforms b15d1f8

Results

Platform Target Build Boot Overall Boot Time Build Logs Boot Logs
Q35 DEBUG ❌ failure ⏩ skipped N/A Build Logs N/A
ArmVirt DEBUG ✅ success ✅ success 0m 13s Build Logs Boot Logs

Workflow run: https://github.com/microsoft/mu_basecore/actions/runs/30954154309

This comment was automatically generated by the Mu QEMU PR Validation workflow.

@kuqin12
kuqin12 force-pushed the mm_comm branch 4 times, most recently from 5c62437 to 6d3b895 Compare August 4, 2026 21:41
kuqin12 added 5 commits August 4, 2026 14:46
…ions

This change adds the SafeIntLib to communication input routine to
validate the incoming MM communication message length before using.

Signed-off-by: Kun Qin <kun.qin@microsoft.com>
…ions

This change adds the SafeIntLib to communication input routine to
validate the incoming MM communication buffer length before using.

Signed-off-by: Kun Qin <kun.qin@microsoft.com>
This change adds the SafeIntLib to communication input routine to
validate the incoming MM communication buffer length before using.

Signed-off-by: Kun Qin <kun.qin@microsoft.com>
…ic operations

This change adds the SafeIntLib to communication input routine to
validate the incoming MM communication buffer length before using.

Signed-off-by: Kun Qin <kun.qin@microsoft.com>
This change adds the SafeIntLib to communication input routine to
validate the incoming MM communication buffer length before using.

Signed-off-by: Kun Qin <kun.qin@microsoft.com>
@kuqin12
kuqin12 marked this pull request as ready for review August 4, 2026 21:56
@codecov-commenter

Codecov Comments Bot (codecov-commenter) commented Aug 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 32 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (release/202511@303f4fa). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...kg/Drivers/StandaloneMmIplPei/StandaloneMmIplPei.c 0.00% 32 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff                @@
##             release/202511   #1846   +/-   ##
================================================
  Coverage                  ?   0.48%           
================================================
  Files                     ?      30           
  Lines                     ?    5558           
  Branches                  ?      30           
================================================
  Hits                      ?      27           
  Misses                    ?    5531           
  Partials                  ?       0           
Flag Coverage Δ
StandaloneMmPkg 0.48% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kuqin12

kuqin12 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

The platform validation is failed due to override hash failure.

Comment on lines +578 to +582
Status = SafeUintnAdd (
EFI_PAGE_SIZE,
mMmCommunicationBuffer->NumberOfPages,
&MaxBufferSize
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this was intended.

Suggested change
Status = SafeUintnAdd (
EFI_PAGE_SIZE,
mMmCommunicationBuffer->NumberOfPages,
&MaxBufferSize
);
Status = SafeUintnMult (
EFI_PAGE_SIZE,
mMmCommunicationBuffer->NumberOfPages,
&MaxBufferSize
);

if (BufferSize <= EFI_PAGES_TO_SIZE (mMmCommunicationBuffer->NumberOfPages)) {
// MU_CHANGE: Use SafeIntLib to safely add the communication header size back to the buffer size if needed
// BufferSize = BufferSize + CommHeaderSize;
Status = SafeUintnAdd (

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This overwrites Status from the MmiManage() call, so the status from the addition is returned instead. You'll probably need to use a second status variable for the addition.

// BufferSize = CommunicateHeader->MessageLength +
// sizeof (CommunicateHeader->HeaderGuid) +
// sizeof (CommunicateHeader->MessageLength);
Status = SafeUintnAdd (

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given CommunicateHeader->MessageLength is UINT64:

typedef struct {
  ///
  /// Allows for disambiguation of the message format.
  ///
  EFI_GUID    HeaderGuid;
  ///
  /// Describes the size of Data (in bytes) and does not include the size of the header.
  ///
  // MU_CHANGE: BZ3398 Make MessageLength the same size in EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
  UINT64      MessageLength;
  ///
  /// Designates an array of bytes that is MessageLength in size.
  ///
  UINT8       Data[1];
} EFI_MM_COMMUNICATE_HEADER;

And BufferSize is UINT64:

typedef struct {
  ///
  /// Indicator GUID for MM core that the communication buffer is compliant with this v3 header.
  /// Must be gEfiMmCommunicateHeaderV3Guid.
  ///
  EFI_GUID    HeaderGuid;
  ///
  /// Describes the size of the entire buffer (in bytes) available for communication, including this communication header.
  ///
  UINT64      BufferSize;
  ///
  /// Reserved for future use.
  ///
  UINT64      Reserved;
  ///
  /// Allows for disambiguation of the message format.
  ///
  EFI_GUID    MessageGuid;
  ///
  /// Describes the size of MessageData (in bytes) and does not include the size of the header.
  ///
  UINT64      MessageSize;
  ///
  /// Designates an array of bytes that is MessageSize in size.
  ///
  UINT8       MessageData[];
} EFI_MM_COMMUNICATE_HEADER_V3;

Was there a reason not to use SafeUint64Add() (and in some similar places)?

MaxBufferSize - BufferSize // MU_CHANGE: Use MaxBufferSize instead of EFI_PAGES_TO_SIZE
);

BufferSize -= CommHeaderSize;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For V3, BufferSize appears to be CommunicateHeader->BufferSize (otherwise it's OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data) + LegacyCommunicateHeader->MessageLength).

In the V3 case, is it guaranteed that BufferSize is >= sizeof (EFI_MM_COMMUNICATE_HEADER_V3 (the value of CommHeaderSize in that case)?

);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "Failed to calculate buffer size: %r\n", Status));
ASSERT (FALSE);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This would be a little more helpful.

Suggested change
ASSERT (FALSE);
ASSERT_EFI_ERROR (Status);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants