Re: [PATCH] MdeModulePkg/PiSmmCore: Cache CommunicationBuffer info before using it

["Yao, Jiewen" <jiewen.yao@intel.com>]

Reviewed-by: Jiewen.yao@intel.com

> -----Original Message-----
> From: Fan, Jeff
> Sent: Friday, November 18, 2016 3:02 PM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Tian, Feng <feng.tian@intel.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: [PATCH] MdeModulePkg/PiSmmCore: Cache CommunicationBuffer
> info before using it
> 
> gSmmCorePrivate->CommunicationBuffer and
> gSmmCorePrivate->BufferSize locate at
> runtime memory region. That means they could be modified by non-SMM
> code during
> runtime.
> 
> We should cache them into SMM local variables before we verify them. After
> verification, we should use the cached ones directly instead of the ones in
> gSmmCorePrivate.
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeff Fan <jeff.fan@intel.com>
> ---
>  MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> index b877a33..de8db65 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> @@ -432,6 +432,8 @@ SmmEntryPoint (
>    EFI_SMM_COMMUNICATE_HEADER  *CommunicateHeader;
>    BOOLEAN                     InLegacyBoot;
>    BOOLEAN                     IsOverlapped;
> +  VOID                        *CommunicationBuffer;
> +  UINTN                       BufferSize;
> 
>    PERF_START (NULL, "SMM", NULL, 0) ;
> 
> @@ -463,17 +465,19 @@ SmmEntryPoint (
>      // Check to see if this is a Synchronous SMI sent through the SMM
> Communication
>      // Protocol or an Asynchronous SMI
>      //
> -    if (gSmmCorePrivate->CommunicationBuffer != NULL) {
> +    CommunicationBuffer = gSmmCorePrivate->CommunicationBuffer;
> +    BufferSize          = gSmmCorePrivate->BufferSize;
> +    if (CommunicationBuffer != NULL) {
>        //
>        // Synchronous SMI for SMM Core or request from Communicate
> protocol
>        //
>        IsOverlapped = InternalIsBufferOverlapped (
> -                       (UINT8 *)
> gSmmCorePrivate->CommunicationBuffer,
> -                       gSmmCorePrivate->BufferSize,
> +                       (UINT8 *) CommunicationBuffer,
> +                       BufferSize,
>                         (UINT8 *) gSmmCorePrivate,
>                         sizeof (*gSmmCorePrivate)
>                         );
> -      if (!SmmIsBufferOutsideSmmValid
> ((UINTN)gSmmCorePrivate->CommunicationBuffer,
> gSmmCorePrivate->BufferSize) || IsOverlapped) {
> +      if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer,
> BufferSize) || IsOverlapped) {
>          //
>          // If CommunicationBuffer is not in valid address scope,
>          // or there is overlap between gSmmCorePrivate and
> CommunicationBuffer,
> @@ -482,19 +486,19 @@ SmmEntryPoint (
>          gSmmCorePrivate->CommunicationBuffer = NULL;
>          gSmmCorePrivate->ReturnStatus = EFI_INVALID_PARAMETER;
>        } else {
> -        CommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER
> *)gSmmCorePrivate->CommunicationBuffer;
> -        gSmmCorePrivate->BufferSize -= OFFSET_OF
> (EFI_SMM_COMMUNICATE_HEADER, Data);
> +        CommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER
> *)CommunicationBuffer;
> +        BufferSize -= OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER,
> Data);
>          Status = SmiManage (
>                     &CommunicateHeader->HeaderGuid,
>                     NULL,
>                     CommunicateHeader->Data,
> -                   &gSmmCorePrivate->BufferSize
> +                   &BufferSize
>                     );
>          //
>          // Update CommunicationBuffer, BufferSize and ReturnStatus
>          // Communicate service finished, reset the pointer to
> CommBuffer to NULL
>          //
> -        gSmmCorePrivate->BufferSize += OFFSET_OF
> (EFI_SMM_COMMUNICATE_HEADER, Data);
> +        gSmmCorePrivate->BufferSize = BufferSize + OFFSET_OF
> (EFI_SMM_COMMUNICATE_HEADER, Data);
>          gSmmCorePrivate->CommunicationBuffer = NULL;
>          gSmmCorePrivate->ReturnStatus = (Status == EFI_SUCCESS) ?
> EFI_SUCCESS : EFI_NOT_FOUND;
>        }
> --
> 2.9.3.windows.2