From 3a146f2a7dca7cc279d752180d4cf728cd78a7b3 Mon Sep 17 00:00:00 2001 From: czhang46 Date: Mon, 15 Apr 2013 01:56:31 +0000 Subject: [PATCH] Fix SMM Variable driver stack GetVariable return INVALID_PARAMETER when DataSize is bigger than SMM communication buffer. Signed-off-by: Chao Zhang Reviewed-by : Dong Guo Reviewed-by : Fu Siyuan git-svn-id: https://edk2.svn.sourceforge.net/svnroot/edk2/trunk/edk2@14276 6f19259b-4bc3-4df7-8a09-765794883524 --- .../RuntimeDxe/VariableSmmRuntimeDxe.c | 38 ++++++++++++++----- .../RuntimeDxe/VariableSmmRuntimeDxe.c | 38 ++++++++++++++----- 2 files changed, 56 insertions(+), 20 deletions(-) diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c index c85d12d951..b83f8c9f4b 100644 --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c @@ -189,6 +189,8 @@ RuntimeServiceGetVariable ( EFI_STATUS Status; UINTN PayloadSize; SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *SmmVariableHeader; + UINTN SmmCommBufPayloadSize; + UINTN TempDataSize; if (VariableName == NULL || VendorGuid == NULL || DataSize == NULL) { return EFI_INVALID_PARAMETER; @@ -198,13 +200,16 @@ RuntimeServiceGetVariable ( return EFI_INVALID_PARAMETER; } - if (*DataSize >= mVariableBufferSize) { - // - // DataSize may be near MAX_ADDRESS incorrectly, this can cause the computed PayLoadSize to - // overflow to a small value and pass the check in InitCommunicateBuffer(). - // To protect against this vulnerability, return EFI_INVALID_PARAMETER if DataSize is >= mVariableBufferSize. - // And there will be further check to ensure the total size is also not > mVariableBufferSize. - // + // + // SMM Communication Buffer max payload size + // + SmmCommBufPayloadSize = mVariableBufferSize - (SMM_COMMUNICATE_HEADER_SIZE + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE); + TempDataSize = *DataSize; + + // + // If VariableName exceeds SMM payload limit. Return failure + // + if (StrSize (VariableName) > SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) { return EFI_INVALID_PARAMETER; } @@ -214,7 +219,14 @@ RuntimeServiceGetVariable ( // Init the communicate buffer. The buffer data size is: // SMM_COMMUNICATE_HEADER_SIZE + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + PayloadSize. // - PayloadSize = OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) + StrSize (VariableName) + *DataSize; + if (TempDataSize > SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - StrSize (VariableName)) { + // + // If output data buffer exceed SMM payload limit. Trim output buffer to SMM payload size + // + TempDataSize = SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - StrSize (VariableName); + } + PayloadSize = OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) + StrSize (VariableName) + TempDataSize; + Status = InitCommunicateBuffer ((VOID **)&SmmVariableHeader, PayloadSize, SMM_VARIABLE_FUNCTION_GET_VARIABLE); if (EFI_ERROR (Status)) { goto Done; @@ -222,7 +234,7 @@ RuntimeServiceGetVariable ( ASSERT (SmmVariableHeader != NULL); CopyGuid (&SmmVariableHeader->Guid, VendorGuid); - SmmVariableHeader->DataSize = *DataSize; + SmmVariableHeader->DataSize = TempDataSize; SmmVariableHeader->NameSize = StrSize (VariableName); if (Attributes == NULL) { SmmVariableHeader->Attributes = 0; @@ -239,7 +251,13 @@ RuntimeServiceGetVariable ( // // Get data from SMM. // - *DataSize = SmmVariableHeader->DataSize; + if (Status == EFI_SUCCESS || Status == EFI_BUFFER_TOO_SMALL) { + // + // SMM CommBuffer DataSize can be a trimed value + // Only update DataSize when needed + // + *DataSize = SmmVariableHeader->DataSize; + } if (Attributes != NULL) { *Attributes = SmmVariableHeader->Attributes; } diff --git a/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmmRuntimeDxe.c b/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmmRuntimeDxe.c index f1111b3b6b..9f750d6780 100644 --- a/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmmRuntimeDxe.c +++ b/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmmRuntimeDxe.c @@ -205,6 +205,8 @@ RuntimeServiceGetVariable ( EFI_STATUS Status; UINTN PayloadSize; SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *SmmVariableHeader; + UINTN SmmCommBufPayloadSize; + UINTN TempDataSize; if (VariableName == NULL || VendorGuid == NULL || DataSize == NULL) { return EFI_INVALID_PARAMETER; @@ -214,13 +216,16 @@ RuntimeServiceGetVariable ( return EFI_INVALID_PARAMETER; } - if (*DataSize >= mVariableBufferSize) { - // - // DataSize may be near MAX_ADDRESS incorrectly, this can cause the computed PayLoadSize to - // overflow to a small value and pass the check in InitCommunicateBuffer(). - // To protect against this vulnerability, return EFI_INVALID_PARAMETER if DataSize is >= mVariableBufferSize. - // And there will be further check to ensure the total size is also not > mVariableBufferSize. - // + // + // SMM Communication Buffer max payload size + // + SmmCommBufPayloadSize = mVariableBufferSize - (SMM_COMMUNICATE_HEADER_SIZE + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE); + TempDataSize = *DataSize; + + // + // If VariableName exceeds SMM payload limit. Return failure + // + if (StrSize (VariableName) > SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) { return EFI_INVALID_PARAMETER; } @@ -230,7 +235,14 @@ RuntimeServiceGetVariable ( // Init the communicate buffer. The buffer data size is: // SMM_COMMUNICATE_HEADER_SIZE + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + PayloadSize. // - PayloadSize = OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) + StrSize (VariableName) + *DataSize; + if (TempDataSize > SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - StrSize (VariableName)) { + // + // If output data buffer exceed SMM payload limit. Trim output buffer to SMM payload size + // + TempDataSize = SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - StrSize (VariableName); + } + PayloadSize = OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) + StrSize (VariableName) + TempDataSize; + Status = InitCommunicateBuffer ((VOID **)&SmmVariableHeader, PayloadSize, SMM_VARIABLE_FUNCTION_GET_VARIABLE); if (EFI_ERROR (Status)) { goto Done; @@ -238,7 +250,7 @@ RuntimeServiceGetVariable ( ASSERT (SmmVariableHeader != NULL); CopyGuid (&SmmVariableHeader->Guid, VendorGuid); - SmmVariableHeader->DataSize = *DataSize; + SmmVariableHeader->DataSize = TempDataSize; SmmVariableHeader->NameSize = StrSize (VariableName); if (Attributes == NULL) { SmmVariableHeader->Attributes = 0; @@ -255,7 +267,13 @@ RuntimeServiceGetVariable ( // // Get data from SMM. // - *DataSize = SmmVariableHeader->DataSize; + if (Status == EFI_SUCCESS || Status == EFI_BUFFER_TOO_SMALL) { + // + // SMM CommBuffer DataSize can be a trimed value + // Only update DataSize when needed + // + *DataSize = SmmVariableHeader->DataSize; + } if (Attributes != NULL) { *Attributes = SmmVariableHeader->Attributes; } -- 2.39.2