Fix SMM Variable driver stack GetVariable return INVALID_PARAMETER when DataSize...

Signed-off-by: Chao Zhang <chao.b.zhang@intel.com>
Reviewed-by  : Dong Guo   <guo.dong@intel.com>
Reviewed-by  : Fu Siyuan  <siyuan.fu@intel.com>

git-svn-id: https://edk2.svn.sourceforge.net/svnroot/edk2/trunk/edk2@14276 6f19259b-4bc3-4df7-8a09-765794883524

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
index c85d12d951364975e0a6f5d357af3564a662edd5..b83f8c9f4b896c83e9173df1c1a62355b1284d2f 100644 (file)
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
@@ -189,6 +189,8 @@ RuntimeServiceGetVariable (
   EFI_STATUS                                Status;\r
   UINTN                                     PayloadSize;\r
   SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE  *SmmVariableHeader;\r
+  UINTN                                     SmmCommBufPayloadSize;\r
+  UINTN                                     TempDataSize;\r
 \r
   if (VariableName == NULL || VendorGuid == NULL || DataSize == NULL) {\r
     return EFI_INVALID_PARAMETER;\r
@@ -198,13 +200,16 @@ RuntimeServiceGetVariable (
     return EFI_INVALID_PARAMETER;\r
   }\r
 \r
-  if (*DataSize >= mVariableBufferSize) {\r
-    //\r
-    // DataSize may be near MAX_ADDRESS incorrectly, this can cause the computed PayLoadSize to\r
-    // overflow to a small value and pass the check in InitCommunicateBuffer().\r
-    // To protect against this vulnerability, return EFI_INVALID_PARAMETER if DataSize is >= mVariableBufferSize.\r
-    // And there will be further check to ensure the total size is also not > mVariableBufferSize.\r
-    //\r
+  //\r
+  // SMM Communication Buffer max payload size\r
+  //\r
+  SmmCommBufPayloadSize = mVariableBufferSize - (SMM_COMMUNICATE_HEADER_SIZE + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE);\r
+  TempDataSize          = *DataSize;\r
+\r
+  //\r
+  // If VariableName exceeds SMM payload limit. Return failure\r
+  //\r
+  if (StrSize (VariableName) > SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) {\r
     return EFI_INVALID_PARAMETER;\r
   }\r
 \r
@@ -214,7 +219,14 @@ RuntimeServiceGetVariable (
   // Init the communicate buffer. The buffer data size is:\r
   // SMM_COMMUNICATE_HEADER_SIZE + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + PayloadSize.\r
   //\r
-  PayloadSize = OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) + StrSize (VariableName) + *DataSize;\r
+  if (TempDataSize > SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - StrSize (VariableName)) {\r
+    //\r
+    // If output data buffer exceed SMM payload limit. Trim output buffer to SMM payload size\r
+    //\r
+    TempDataSize = SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - StrSize (VariableName);\r
+  }\r
+  PayloadSize = OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) + StrSize (VariableName) + TempDataSize;\r
+\r
   Status = InitCommunicateBuffer ((VOID **)&SmmVariableHeader, PayloadSize, SMM_VARIABLE_FUNCTION_GET_VARIABLE);\r
   if (EFI_ERROR (Status)) {\r
     goto Done;\r
@@ -222,7 +234,7 @@ RuntimeServiceGetVariable (
   ASSERT (SmmVariableHeader != NULL);\r
 \r
   CopyGuid (&SmmVariableHeader->Guid, VendorGuid);\r
-  SmmVariableHeader->DataSize   = *DataSize;\r
+  SmmVariableHeader->DataSize   = TempDataSize;\r
   SmmVariableHeader->NameSize   = StrSize (VariableName);\r
   if (Attributes == NULL) {\r
     SmmVariableHeader->Attributes = 0;\r
@@ -239,7 +251,13 @@ RuntimeServiceGetVariable (
   //\r
   // Get data from SMM.\r
   //\r
-  *DataSize = SmmVariableHeader->DataSize;\r
+  if (Status == EFI_SUCCESS || Status == EFI_BUFFER_TOO_SMALL) {\r
+    //\r
+    // SMM CommBuffer DataSize can be a trimed value\r
+    // Only update DataSize when needed\r
+    //\r
+    *DataSize = SmmVariableHeader->DataSize;\r
+  }\r
   if (Attributes != NULL) {\r
     *Attributes = SmmVariableHeader->Attributes;\r
   }\r

diff --git a/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmmRuntimeDxe.c b/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmmRuntimeDxe.c
index f1111b3b6b3d6fbd4b0b731554f6d200be569a79..9f750d678068904d71253823592aaa03551e9486 100644 (file)
--- a/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmmRuntimeDxe.c
+++ b/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmmRuntimeDxe.c
@@ -205,6 +205,8 @@ RuntimeServiceGetVariable (
   EFI_STATUS                                Status;\r
   UINTN                                     PayloadSize;\r
   SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE  *SmmVariableHeader;\r
+  UINTN                                     SmmCommBufPayloadSize;\r
+  UINTN                                     TempDataSize;\r
 \r
   if (VariableName == NULL || VendorGuid == NULL || DataSize == NULL) {\r
     return EFI_INVALID_PARAMETER;\r
@@ -214,13 +216,16 @@ RuntimeServiceGetVariable (
     return EFI_INVALID_PARAMETER;\r
   }\r
 \r
-  if (*DataSize >= mVariableBufferSize) {\r
-    //\r
-    // DataSize may be near MAX_ADDRESS incorrectly, this can cause the computed PayLoadSize to\r
-    // overflow to a small value and pass the check in InitCommunicateBuffer().\r
-    // To protect against this vulnerability, return EFI_INVALID_PARAMETER if DataSize is >= mVariableBufferSize.\r
-    // And there will be further check to ensure the total size is also not > mVariableBufferSize.\r
-    //\r
+  //\r
+  // SMM Communication Buffer max payload size\r
+  //\r
+  SmmCommBufPayloadSize = mVariableBufferSize - (SMM_COMMUNICATE_HEADER_SIZE + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE);\r
+  TempDataSize          = *DataSize;\r
+\r
+  //\r
+  // If VariableName exceeds SMM payload limit. Return failure\r
+  //\r
+  if (StrSize (VariableName) > SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) {\r
     return EFI_INVALID_PARAMETER;\r
   }\r
 \r
@@ -230,7 +235,14 @@ RuntimeServiceGetVariable (
   // Init the communicate buffer. The buffer data size is:\r
   // SMM_COMMUNICATE_HEADER_SIZE + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + PayloadSize.\r
   //\r
-  PayloadSize = OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) + StrSize (VariableName) + *DataSize;\r
+  if (TempDataSize > SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - StrSize (VariableName)) {\r
+    //\r
+    // If output data buffer exceed SMM payload limit. Trim output buffer to SMM payload size\r
+    //\r
+    TempDataSize = SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - StrSize (VariableName);\r
+  }\r
+  PayloadSize = OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) + StrSize (VariableName) + TempDataSize;\r
+\r
   Status = InitCommunicateBuffer ((VOID **)&SmmVariableHeader, PayloadSize, SMM_VARIABLE_FUNCTION_GET_VARIABLE);\r
   if (EFI_ERROR (Status)) {\r
     goto Done;\r
@@ -238,7 +250,7 @@ RuntimeServiceGetVariable (
   ASSERT (SmmVariableHeader != NULL);\r
 \r
   CopyGuid (&SmmVariableHeader->Guid, VendorGuid);\r
-  SmmVariableHeader->DataSize   = *DataSize;\r
+  SmmVariableHeader->DataSize   = TempDataSize;\r
   SmmVariableHeader->NameSize   = StrSize (VariableName);\r
   if (Attributes == NULL) {\r
     SmmVariableHeader->Attributes = 0;\r
@@ -255,7 +267,13 @@ RuntimeServiceGetVariable (
   //\r
   // Get data from SMM.\r
   //\r
-  *DataSize = SmmVariableHeader->DataSize;\r
+  if (Status == EFI_SUCCESS || Status == EFI_BUFFER_TOO_SMALL) {\r
+    //\r
+    // SMM CommBuffer DataSize can be a trimed value\r
+    // Only update DataSize when needed\r
+    //\r
+    *DataSize = SmmVariableHeader->DataSize;\r
+  }\r
   if (Attributes != NULL) {\r
     *Attributes = SmmVariableHeader->Attributes;\r
   }\r