Improving error handing in Variable Smm#12670
Conversation
Some platforms may disable `ASSERT_DEADLOOP_ENABLED` in `PcdDebugPropertyMask`. In this case ASSERT won't hang the machine. Replaced those simple ASSERT with proper error returning handling. Signed-off-by: Paddy Deng (AMI US Holdings Inc) <v-dengpaddy@microsoft.com>
Some control flow in SmmVariableHandler rely on the global variable mNvVariableCache. But without checking if its value is valid or not. This commit adds several null guard before the handler accessing the cache variable. Signed-off-by: Paddy Deng (AMI US Holdings Inc) <v-dengpaddy@microsoft.com>
PaddyDeng-v
force-pushed
the
user/paddydeng/HardenVariableSmm
branch
from
88d5d11 to
5b559c9
Compare
| &mSmmVarCheck | ||
| ); | ||
| ASSERT_EFI_ERROR (Status); | ||
| if (EFI_ERROR (Status)) { |
There was a problem hiding this comment.
@PaddyDeng-v, I think you need to consider system-state cleanup since this is a behavior change. Previously, an assertion call either:
- Did nothing
- Triggered a breakpoint
- Entered a deadloop
This change adds a new possibility to return partially through the initialization flow at various points to firmware execution.
If subsequent initialization fails, gEfiSmmVariableProtocolGuid should likely be uninstalled as it exposes MM variable services within the MM environment that would be in a "partially initialized" state.
For example, if gEfiSmmVariableProtocolGuid is installed successfully (Line 1179), but gEdkiiSmmVarCheckProtocolGuid installation fails (Line 1196), that means the call to register the variable SMI handler (Line 1237) is not executed. MM drivers (using gEfiSmmVariableProtocolGuid directly) might participate in a flow where they expect variables written from outside the MM environment (which relies on the SMI handler) and that is not available.
These types of dependencies can be established and broken down throughout initialization, but I think the simplest answer is that "if initialization fails, variable services are not available". So, interfaces in global databases (like the protocol database) are removed.
| SmmVariableFunctionHeader = (SMM_VARIABLE_COMMUNICATE_HEADER *)CommBuffer; | ||
| switch (SmmVariableFunctionHeader->Function) { | ||
| case SMM_VARIABLE_FUNCTION_GET_VARIABLE: | ||
| if (mNvVariableCache == NULL) { |
There was a problem hiding this comment.
mNvVariableCache is assigned in InitNonVolatileVariableStore() via VariableCommonInitialize(), which runs at the very top of MmVariableServiceInitialize. Since commit 1 now aborts before MmiHandlerRegister when that fails, the SMI handler should only ever be registered when mNvVariableCache is non-NULL, so these per-case NULL checks look unreachable in the normal flow. Are they guarding a specific scenario (e.g. a custom/downstream init ordering)? If so, a brief comment would help future readers; if not, they could be dropped, since commit 1 is the actual safeguard
|
|
||
| if (CommBufferPayloadSize < OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) { | ||
| DEBUG ((DEBUG_ERROR, "GetVariable: SMM communication buffer size invalid!\n")); | ||
| return EFI_SUCCESS; |
There was a problem hiding this comment.
Small consistency note: the new guards correctly set Status and goto EXIT (so ReturnStatus is written back), but the sibling buffer-size checks in these cases still return EFI_SUCCESS without setting ReturnStatus. Because the DXE caller reads SmmVariableFunctionHeader->ReturnStatus directly, that path can surface a stale value from the previous operation. Converting these to Status = EFI_INVALID_PARAMETER; goto EXIT; would make the handler consistent. (Pre-existing; optional, but you're already in these cases.)
3 participants
Description
This PR includes 2 commits that improves error handling in VariableSmm
MmVariableServiceInitialize: Some platform may disable dead loop on assert. In this case, we should also abort the function in assert cases.mNvVariableCachebefore use in SmiVariableHandlerHow This Was Tested
After apply both commits, the platform can still boot to OS. No error is happing.
Integration Instructions
N/A