[PATCH] MdeModulePkg VariableSmm: Do not need check CommBufferSize buffer

[PATCH] MdeModulePkg VariableSmm: Do not need check CommBufferSize buffer

[Star Zeng]

Current code in SmmVariableHandler() checks CommBufferSize
buffer to make sure it points to outside SMRAM in
"case SMM_VARIABLE_FUNCTION_GET_STATISTICS".

But after eaae7b33b1cf6b9f21db1636f219c2b6a8d88afd,
CommBufferSize buffer points to SMRAM that was used by
SMM core to cache CommSize from SmmCommunication protocol,
then the check will fail definitely and GET_STATISTICS
feature breaks.

In fact, do not need check CommBufferSize buffer at all
even before eaae7b33b1cf6b9f21db1636f219c2b6a8d88afd.
Before eaae7b33b1cf6b9f21db1636f219c2b6a8d88afd,
CommBufferSize buffer pointed to gSmmCorePrivate->BufferSize
that is outside SMRAM, the check will success definitely;
after eaae7b33b1cf6b9f21db1636f219c2b6a8d88afd,
CommBufferSize buffer points to local variable BufferSize
(in SMRAM) in SmmEntryPoint(), the check is not needed
definitely.

The patch is to remove the check.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jeff Fan <jeff.fan@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
index eafb53322e8c..c714916019ef 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
@@ -695,11 +695,10 @@ SmmVariableHandler (
       // It is covered by previous CommBuffer check
       //
 
-      if (!SmmIsBufferOutsideSmmValid ((EFI_PHYSICAL_ADDRESS)(UINTN)CommBufferSize, sizeof(UINTN))) {
-        DEBUG ((EFI_D_ERROR, "GetStatistics: SMM communication buffer in SMRAM!\n"));
-        Status = EFI_ACCESS_DENIED;
-        goto EXIT;
-      }
+      //
+      // Do not need to check CommBufferSize buffer as it should point to SMRAM
+      // that was used by SMM core to cache CommSize from SmmCommunication protocol.
+      //
 
       Status = SmmVariableGetStatistics (VariableInfo, &InfoSize);
       *CommBufferSize = InfoSize + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE;
-- 
2.7.0.windows.1

Re: [PATCH] MdeModulePkg VariableSmm: Do not need check CommBufferSize buffer

[Yao, Jiewen]

Reviewed-by: Jiewen.yao@intel.com


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Star
> Zeng
> Sent: Friday, December 9, 2016 2:53 PM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Fan, Jeff <jeff.fan@intel.com>; Zeng,
> Star <star.zeng@intel.com>
> Subject: [edk2] [PATCH] MdeModulePkg VariableSmm: Do not need check
> CommBufferSize buffer
> 
> Current code in SmmVariableHandler() checks CommBufferSize buffer to make
> sure it points to outside SMRAM in "case
> SMM_VARIABLE_FUNCTION_GET_STATISTICS".
> 
> But after eaae7b33b1cf6b9f21db1636f219c2b6a8d88afd,
> CommBufferSize buffer points to SMRAM that was used by SMM core to cache
> CommSize from SmmCommunication protocol, then the check will fail definitely
> and GET_STATISTICS feature breaks.
> 
> In fact, do not need check CommBufferSize buffer at all even before
> eaae7b33b1cf6b9f21db1636f219c2b6a8d88afd.
> Before eaae7b33b1cf6b9f21db1636f219c2b6a8d88afd,
> CommBufferSize buffer pointed to gSmmCorePrivate->BufferSize that is outside
> SMRAM, the check will success definitely; after
> eaae7b33b1cf6b9f21db1636f219c2b6a8d88afd,
> CommBufferSize buffer points to local variable BufferSize (in SMRAM) in
> SmmEntryPoint(), the check is not needed definitely.
> 
> The patch is to remove the check.
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jeff Fan <jeff.fan@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> index eafb53322e8c..c714916019ef 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> @@ -695,11 +695,10 @@ SmmVariableHandler (
>        // It is covered by previous CommBuffer check
>        //
> 
> -      if (!SmmIsBufferOutsideSmmValid
> ((EFI_PHYSICAL_ADDRESS)(UINTN)CommBufferSize, sizeof(UINTN))) {
> -        DEBUG ((EFI_D_ERROR, "GetStatistics: SMM communication buffer in
> SMRAM!\n"));
> -        Status = EFI_ACCESS_DENIED;
> -        goto EXIT;
> -      }
> +      //
> +      // Do not need to check CommBufferSize buffer as it should point to
> SMRAM
> +      // that was used by SMM core to cache CommSize from
> SmmCommunication protocol.
> +      //
> 
>        Status = SmmVariableGetStatistics (VariableInfo, &InfoSize);
>        *CommBufferSize = InfoSize +
> SMM_VARIABLE_COMMUNICATE_HEADER_SIZE;
> --
> 2.7.0.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel