microsoft / mu_feature_mm_supv

Project Mu - Feature Repo - MM Supervisor

Project Mu MM Supervisor Repository

Host Type & Toolchain	Build Status	Test Status	Code Coverage
Windows_VS
Ubuntu_GCC5

This repository is part of Project Mu. Please see Project Mu for details https://microsoft.github.io/mu

This MM Supervisor feature repo contains the supervisor module under Standalone MM environment for X64 architecture, refactored from TianoCore common modules and public portion of AMD SMM supervisor module. The repo intends to support operating Standalone MM modules in a secure manner. Other peripheral libraries are also included to accomodate user module operations.

Detailed Feature Information

Far more details about using this repo can be found in: MM Supervisor Design.

Repository Philosophy

Like other Project MU feature repositories, the Project MU MM Supervisor feature repo does not strictly follow the EDKII releases, but instead has a continuous main branch which will periodically receive cherry-picks of needed changes from EDKII. For stable builds, release tags will be used instead to determine commit hashes at stable points in development. Release branches may be created as needed to facilitate a specific release with needed features, but this should be avoided.

Consuming the MM Supervisor Feature Package

Since this project does not follow the release fork model, the code should be consumed from a release hash and should be consumed as a extdep in the platform repo. To include, create a file named feature_mm_supv_ext_dep.yaml desired release tag hash. This could be in the root of the project or in a subdirectory as desired.

{

"scope": "global",

"type": "git",

"name": "FEATURE_MM_SUPV",

"var_name": "FEATURE_MM_SUPV_PATH",

"source": "https://github.com/microsoft/mu_feature_mm_supv.git",

"version": "<RELEASE HASH>",

"flags": ["set_build_var"]

}

Setting the the var_name and the set_build_var flags will allow the build scripts to reference the extdep location. To make sure that the package is discoverable for the build, the following line should also be added to the build MM supervisor GetPackagesPath list.

shell_environment.GetBuildVars().GetValue("FEATURE_MM_SUPV_PATH", "")

Note: If using pytool extensions older then version 0.17.0 you will need to append the root path to the build variable string.

After this the package should be discoverable to can be used in the build like any other dependency.

Code of Conduct

This project has adopted the Microsoft Open Source Code of Conduct https://opensource.microsoft.com/codeofconduct/

For more information see the Code of Conduct FAQ https://opensource.microsoft.com/codeofconduct/faq/ or contact [email protected]. with any additional questions or comments.

Contributions

Contributions are always welcome and encouraged! Please open any issues in the Project Mu GitHub tracker and read https://microsoft.github.io/mu/How/contributing/

• [Code Requirements](https://microsoft.github.io/mu/CodeDevelopment/requirements/)
• [Doc Requirements](https://microsoft.github.io/mu/DeveloperDocs/requirements/)

Issues

Please open any issues in the Project Mu GitHub tracker. [More Details](https://microsoft.github.io/mu/How/contributing/)

Builds

Please follow the steps in the Project Mu docs to build for CI and local testing. [More Details](https://microsoft.github.io/mu/CodeDevelopment/compile/)

Copyright & License

Files in this repository have their own copyright. Otherwise, the following copyrights applies.

Copyright (C) Microsoft Corporation
SPDX-License-Identifier: BSD-2-Clause-Patent

Upstream License (TianoCore) ===================

Copyright (c) 2019, TianoCore and contributors. All rights reserved.

SPDX-License-Identifier: BSD-2-Clause-Patent

Redistribution and use in source and binary forms, with or without modification, are permitted provided that the following conditions are met:

1. Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer.
2. Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the following disclaimer in the documentation and/or other materials provided with the distribution.

Subject to the terms and conditions of this license, each copyright holder and contributor hereby grants to those receiving rights under this license a perpetual, worldwide, non-exclusive, no-charge, royalty-free, irrevocable (except for failure to satisfy the conditions of this license) patent license to make, have made, use, offer to sell, sell, import, and otherwise transfer this software, where such license applies only to those patent claims, already acquired or hereafter acquired, licensable by such copyright holder or contributor that are necessarily infringed by:

1. their Contribution(s) (the licensed copyrights of copyright holders and non-copyrightable additions of contributors, in source or binary form) alone; or
2. combination of their Contribution(s) with the work of authorship to which such Contribution(s) was added by such copyright holder or contributor, if, at the time the Contribution is added, such addition causes such combination to be necessarily infringed. The patent license shall not apply to any other combinations which include the Contribution.

Except as expressly stated above, no rights or licenses from any copyright holder or contributor is granted under this license, whether expressly, by implication, estoppel or otherwise.

DISCLAIMER

THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDERS OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

mu_feature_mm_supv's Issues

Unvalidated CpuIndex (Arg2) in SMM_START_AP_PROC can cause out of bound writes

The supervisor handles the SMM_START_AP_PROC call index. It calls the gMmCoreMmst.MmStartupThisAp() function pointer with 3 arguments provided by userspace. The exact function pointer points to SmmStartupThisAp().

The second argument (Arg2) is seen as a CpuIndex and is used as an index into an array that is being written too. There are no bounds checks to ensure the index is <= the number of CPUs present. In this case, user-space gets to provide an unvalidated index and write user-controlled values almost anywhere in memory.

Fix Recommendation:
Add CpuIndex validation, either to SmmStartupThisAp() or inside the dispatcher when handling SMM_START_AP_PROC.

Acknowledgement:
Thanks to @iljavs for reporting this issue.

SMM_UNREG_HNDL can unregister any handler, including supervisor ones

The supervisor handles syscalls to register and unregister handlers on user space's behalf by exposing a SMM_REG_HNDL and SMM_UNREG_HNDL syscall. for registration it calls MmiUserHandlerRegister() to do this work, which calls CoreMmiHandlerRegister() with IsSupervisorHandler set to FALSE. For unregistering, it calls MmiHandlerUnRegister() with a user-provided handle. It performs a handle lookup and, if found, unregisters the handler. MmiHandlerUnRegister() makes no effort to see whether the handler's IsSupervisor BOOLEAN is set or not. In order words, a user-space caller can unregister a supervisor-created handler.

The impact of unregistering supervisor handler could potentially miss some events and potentially lose capability of reporting secure policy. However, security critical events will be enforced by the time external entities fetch the policy, or the OS will indicate system guard is off.

Fix recommendation:
Added a MmiUserHandlerUnRegister() function that makes sure only handlers with their IsSupervisor field set to FALSE can be unregistered through the SMM_UNREG_HNDL syscall.

Acknowledgement:
Thanks to @iljavs for reporting this issue.

[Feature]: Adopt semantic versioning

Feature Overview

The request is to adopt semantic versioning in this repository.

This is similar to other Project Mu repos that have such as:

• mu_basecore
• mu_feature_dfci
• mu_tiano_platforms
• mu_devops

This allows consumers to more easily ingest releases per the goals of the semantic versioning specification.

It also allows workflows in Mu DevOps to operate on the repo such as:

• Release Drafter
• Submodule Version Updater

Solution Overview

Switch repository versioning to comply with the Semantic Versioning 2.0.0 Specification.

Alternatives Considered

Keep as-is. This has the disadvantage of inconsistent versioning limiting compatibility with tools and consuming processes.

Urgency

Low

Are you going to implement the feature request?

Someone else needs to implement the feature

Do you need maintainer feedback?

Maintainer feedback requested

Anything else?

No response

[Feature]: Control Syscall Enter/Exit Message Separately from DEBUG_VERBOSE

Feature Overview

The SysCallDispatcher() function in MmSupervisorPkg/Core/PrivilegeMgmt/SyscallDispatcher.c prints a message upon every entry and exit to the function.

mu_feature_mm_supv/MmSupervisorPkg/Core/PrivilegeMgmt/SyscallDispatcher.c

Lines 245 to 256 in 8b8aeef

	DEBUG ((
	DEBUG_VERBOSE,
	"%a Enter... CallIndex: %lx, Arg1: %lx, Arg2: %lx, Arg3: %lx, CallerAddr: %p, Ring3Stack %p\n",
	__FUNCTION__,
	CallIndex,
	Arg1,
	Arg2,
	Arg3,
	CallerAddr,
	Ring3StackPointer
	));

The print error level is DEBUG_VERBOSE but the message frequency greatly exceeds the number of messages typically printed in a single function at that level. It effectively makes a boot with DEBUG_VERBOSE level set impossible to use in a reasonable developer cycle.

On one system, it has printed 86,000+ times on a single boot (before being powered off). On the UART at 115200, this has consumed 39+ minutes and the text dominates the serial log.

This is useful information, but it's not general verbose information. It's extreme detail for a very specific scenario that I believe is best handled with a special flag.

This request does not specify how this flag is implemented.

Note: This is submitted as a feature request, but could also be considered a bug.

Solution Overview

A specific solution is not given. Options could include:

• Define a print error level for "MM Supervisors" (or something more relevant) in gEfiMdePkgTokenSpaceGuid.PcdFixedDebugPrintErrorLevel

• Define a print error level PCD in 'MmSupervisorPkg` and add a bit for the sys call dispatcher

• Define another mechanism like a dedicated (e.g. boolean) PCD to control whether this message is printed

Alternatives Considered

1. Override the PrintErrorLevel on the MmSupervisorCore module to remove DEBUG_VERBOSE

Not desirable as other verbose messages in the module are missed.

2. Comment out this message.

Not desirable since source code modification should not be necessary for this specific case.

Urgency

Medium

Are you going to implement the feature request?

Someone else needs to implement the feature

Do you need maintainer feedback?

Maintainer feedback requested

Anything else?

No response

Missing device validation in ConvertSmiHandlerUsbContext()

ConvertSmiHandlerUsbContext() is called by SmiHandlerProfileRegisterHandler() which is called by ProcessUserHandlerReg() when the supervisor dispatcher handles a SMM_MM_HDL_REG_1/SMM_MM_HDL_REG_2 request to register a child SMI handler with a gEfiSmmUsbDispatch2ProtocolGuid handler guid. ConvertSmiHandlerUsbContext() is handed a user space provided context and context size, and it will convert an EFI_SMM_USB_REGISTER_CONTEXT structure to a SMI_HANDLER_PROFILE_USB_REGISTER_CONTEXT structure. An EFI_SMM_USB_REGISTER_CONTEXT structure contains a device pointer and a type. ConvertSmiHandlerUsbContext() will call GetDevicePathSize() to get the device path size and then allocate a SMI_HANDLER_PROFILE_USB_REGISTER_CONTEXT with enough room for the device and copy it over into the structure.

The problem here is that UsbContext->Device is a user-space provided pointer to a variable-length structure that more or less acts as a sort of linked list (but instead of next pointers, it has 16-bit length fields) of device elements, where each element has a type, subtype a length and content. GetDevicePathSize() has no idea it runs inside the supervisor and has no concept of user-space provided pointers. This is a problem. It doesn't call InspectTargetRangeOwnership(); furthermore, even if it did, the length could change after the call to InspectTargetRangeOwnership() because the pointer comes from user-space (who might be able to change the data asynchronously, either through a 2nd core or by utilizing async DMA or async IO) or even the device pointer could change. Additionally, there is no check to make sure the UsbContext size is the size of an EFI_SMM_USB_REGISTER_CONTEXT structure; there is just an assert.

Fix Recommendation:

• on entry of the function, make sure UsbContextSize is at least sizeof (EFI_SMM_USB_REGISTER_CONTEXT)
• capture the device pointer
• InspectTargetRangeOwnership on the captured device pointer, with a size of sizeof(EFI_DEVICE_PATH_PROTOCOL)
• have to implement your own GetDevicePathSize()

Acknowledgement:
Thanks to @iljavs for reporting this issue.

Global length field updated even allocation in MmInstallConfigurationTable() failed

MmInstallConfigurationTable() is called by the supervisor to allow user space to install configuration tables. It looks up the guid, and if it can't find it in the current table, it adds an entry. If there is no room left in the tables, they get reallocated to account for extra space. This reallocation has an interesting logic error. It updates the global variable (mMmSystemTableAllocateSize) that holds the table's max size prior to allocation. If the allocation fails (and hence no reallocation occurs), the global size variable isn't reset. This means the next caller that wants to add an entry would trigger memory corruption.

This has the potential for a compromised SMM handler to break into the supervisor, assuming a full memory corruption exploit. One of the prerequisites would be getting AllocatePool() to fail; however, I suspect this could be done utilizing a memory leak.

Fix recommendation:
This could be fixed by either resetting mMmSystemTableAllocateSize on allocation failure or by using a temporary size value before allocation and assigning it to mMmSystemTableAllocateSize after successful allocation.

Acknowledgement:
Thanks to @iljavs for reporting this issue.

MmFreePages() doesn't have the same NumberOfPages check that MmInternalAllocatePagesEx() has (SMM_FREE_PAGE)

The supervisor calls MmFreePages() to free pages previously allocated with a call to SMM_ALOC_PAGE. In the SMM_ALOC_PAGE case, a call is made to MmAllocatePages(), which eventually lands inside of MmInternalAllocatePagesEx(), which makes sure the number of pages provided by user space can't be too large (no more than 48 bits essentially, as any more pages would require more memory than the 64-bit address space would logically allow for). This check makes a lot of sense.

Unfortunately, MmAllocatePages()'s counterpart, MmFreePages() has no such check. The number of pages provided by userland callers can be > 48 bits big. When converted to actual size (using the EFI_PAGES_TO_SIZE() macro), an integer overflow can lead the code to believe that fewer pages than required are being freed.

If the heap guard is enabled and the memory is guarded, in that case, ClearGuardedMemoryBits() loops over a bitmap based on the number of pages (without using the EFI_PAGES_TO_SIZE() macro). The worst-case scenario would be some kind of memory corruption.

Fix recommendation:
Adding a check like MmInternalAllocatePagesEx() has. And add an overflow check for user page free call in the syscall dispatcher.

Acknowledgement:
Thanks to @iljavs for reporting this issue.

Potential integer overflow in IsIoReadWriteAllowed()

The supervisor calls IsIoReadWriteAllowed() to see if a user can read or write to specific io ports. The code looks pretty decent; however, a few minor things seem wrong. There's some weirdness around casting and truncating when calling it (and when later using the ioports), which looks kinda bad, but ultimately it doesn't seem to cause much problems.

When validating the ioport and size, there is an integer overflow. It will overflow if the port is really big (0xfffffffd or larger). If there is a policy to allow writes to low ports (e.g., 0), then it could allow writing to a high port (e.g., 65535, 0xffffffff truncated to 16 bits). Ultimately this is a really specific set of circumstances and probably not that big of a deal, but it does look like a clear bug that should be addressed.

Fix recommendation:
A safe int function to convert port (IoAddress) to prevent any integer overflow from occurring. Also added an integer overflow check as a defense in depth mechanism.

Acknowledgement:
Thanks to @iljavs for reporting this issue.

[Feature]: Perf logging in Standalone MM

Feature Overview

This ticket is created to request the performance logging inside Standalone MM. Currently the build is linked to NULL instances, which provides 0 performance logging in Standalone MM.

Solution Overview

This change should involve a series of PerformanceLib instances for core and user mode drivers, corresponding framework updates to collect logging from standalone MM, memory management update for common area used by performance driver and the supervisor core.

Alternatives Considered

No response

Urgency

Low

Are you going to implement the feature request?

I will implement the feature

Do you need maintainer feedback?

Maintainer feedback requested

Anything else?

No response

[Feature]: Fail fast for MM supervisor

Feature Overview

This change is requested to support collecting OS stacks when a supervisor detected rogue syscall intentions, which enables the system to collect more context when the issue happens to troubleshoot the issue more facilely.

An example is that when an OS driver triggered SMI with IO read, which expected the SMM to update save state. However, some firmware entities will still try to read certain save states, which is not allowed according to Windows Secured Core PC specification.

This feature should help to identify the OS driver call stack to coordinate the fix faster.

Solution Overview

The Windows system provided a method to read error information from GHES ACPI table and record the error to system log when an NMI is detected.

The idea here is to inject an NMI from SMM when syscall dispatcher failed with access denies, which should be caught by the OS when returning to normal DXE environment. Supervisor can populate the GHES with needed information such as RIP and driver name, etc. for SMM context.

Alternatives Considered

• Custom ways to record the error in a static memory, then use a customized driver to read such information.
  • Too much customization and not able to scale
• Tamper the save state, i.e. nullify the RIP from save state, which will also cause a page fault
  • This can be used a debugging technique, but for a legitimate solution, one needs to properly propagate the error message. What is more, the NMI will have the capability to read from GHES, which is the resource field SMM can populate information to.

Urgency

Low

Are you going to implement the feature request?

I will implement the feature

Do you need maintainer feedback?

No maintainer feedback needed

Anything else?

No response

This repo is missing important files

There are important files that Microsoft projects should all have that are not present in this repository. A pull request has been opened to add the missing file(s). When the pr is merged this issue will be closed automatically.

Microsoft teams can learn more about this effort and share feedback within the open source guidance available internally.

Merge this pull request

Race condition in SmiHandlerProfileUnregisterHandler() that could lead to FreePool() of something that isn't pool allocated

SmiHandlerProfileUnregisterHandler() is called by ProcessUserHandlerUnreg() which is called by the supervisor when handling SMM_MM_HDL_UNREG_1/SMM_MM_HDL_UNREG_2 syscalls. It takes a guid pointer (as well as a context and context size) provided by user space. It has been inspected with a call to InspectTargetRangeOwnership() to ensure it belongs to user space. SmiHandlerProfileUnregisterHandler() uses the guid pointer twice. Once to look up the handler (and its associated context) and a second time to free memory if the guid content is the same as gEfiSmmUsbDispatch2ProtocolGuid.

Given that the guid points to user space controlled data, it is possible that this content could have changed between the first time it was used and the second time it was used. I'm not sure if user space gets access to more than one core (in which case they could instruct the 2nd core to modify the data asynchronously). If the guid content is not equal to gEfiSmmUsbDispatch2ProtocolGuid during the first use but is equal during the second use, user space can induce the supervisor to FreePool() data that user space controls instead of supervisor allocated pool memory.

In addition, MmInstallProtocolInterfaceNotify() only has an assert (this is reachable through the SMM_INST_PROT syscall with a similar user-provided guid pointer) that could be triggered with a similar guid race where multiple entries in a list could have the same guid. But this is benign because it occurs after initial MM launching and does not impact supervisor behavior.

One last note, SmiHandlerProfileUnregisterHandler() has a second case that compares against a specific guid (gEfiSmmSwDispatch2ProtocolGuid) and returns a supervisor pool allocated piece of memory. However, in this case, there is no logic to free this memory at the end of the function, leading to a memory leak. In and on itself, memory leaks aren't great, but (certainly in the context of UEFI/SMM) I wouldn't call them out as a security issue. However, exploitation of the "MmInstallConfigurationTable()" issue described above would likely benefit from a reliable memory leak, and as such, this might have some security consequences.

Fix Recommendation:

• Consider capturing the user-provided guid before using it.
• Fix the memory leak.

Acknowledgement:
Thanks to @iljavs for reporting this issue.

Syscall entry/exit register save / restore (general purpose and FPU/MMX/SSE/AVX)

SyscallCenter implemented in SysCallEntry.nasm is called as the supervisor entry point when user space issues a SYSCALL instruction. This function is responsible for proper setup, calling the dispatcher, and then returning to user space properly.

Except, it doesn't seem to restore all registers. It restores a subset of the general-purpose registers (r11, rbx, rdi, r12, rsi, r9, r8, rdx, rbp rcx and rsp), clearly leaving out r10, r13, r14, and r15. If the dispatcher uses any of these registers, they would get returned to user space.

In addition to the general-purpose registers, it doesn't seem to restore any of the FPU/MMX/SSE/AVX registers. Optimizing compilers tend to use the SIMD registers and instructions for things like memcpy().

Fix Recommendation:
Consider saving/restoring all general-purpose registers.

Acknowledgement:
Thanks to @iljavs for reporting this issue.

Fix CodeQL alert (MmSupervisorPkg/Core/Request/RequestDispatcher.c)- Redundant null check due to previous dereference

Tracking issue for:

• https://github.com/microsoft/mu_feature_mm_supv/security/code-scanning/10

[Bug]: Fragmented runtime buffer due to MM PEI initialization

Is there an existing issue for this?

• I have searched existing issues

Current Behavior

The PEI initialization would require communicate buffer to be allocated during PEI phase and unblocked by supervisor. This is causing the system to have fragmented memory map on the runtime memory type.

Expected Behavior

The runtime memory should hold consistent and not fragmented.

Steps To Reproduce

Run the test point toolkit, it will report error.

Build Environment

- OS(s):
- Tool Chain(s):
- Targets Impacted:

Version Information

7.002 or lower.

Urgency

Medium

Are you going to fix this?

I will fix it

Do you need maintainer feedback?

Maintainer feedback requested

Anything else?

No response

Missing length check in ConvertSmiHandlerSwContext()

The supervisor handles syscalls to register and unregister handlers on user space's behalf by exposing a SMM_REG_HNDL and SMM_UNREG_HNDL syscall. for registration it calls MmiUserHandlerRegister() to do this work, which calls CoreMmiHandlerRegister() with IsSupervisorHandler set to FALSE. For unregistering, it calls MmiHandlerUnRegister() with a user-provided handle. It performs a handle lookup and, if found, unregisters the handler. MmiHandlerUnRegister() makes no effort to see whether the handler's IsSupervisor BOOLEAN is set or not. In order words, a user-space caller can unregister a supervisor-created handler.

The impact of unregistering supervisor handler could potentially miss some events and potentially lose capability of reporting secure policy. However, security critical events will be enforced by the time external entities fetch the policy, or the OS will indicate system guard is off.

Fix Recommendation:
Consider turning the ASSERT length check into a proper length check (so that it would also trigger in release builds).

Acknowledgement:
Thanks to @iljavs for reporting this issue.