1. Patchew
  2. EDK2
  3. [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER
  4. View patch

[edk2-devel] [PATCH v1 2/5] MdeModulePkg: PiSmmIpl: Update MessageLength calculation for MmCommunicate

Kun Qin posted 5 patches 4 years, 8 months ago
There is a newer version of this series
[edk2-devel] [PATCH v1 2/5] MdeModulePkg: PiSmmIpl: Update MessageLength calculation for MmCommunicate
Posted by Kun Qin 4 years, 8 months ago 
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3398

This change updated calculation routine for MM communication in PiSmmIpl.
It removes ambiguity brought in by UINTN variables from this routine and
paves way for updating definition of field MessageLength in
EFI_MM_COMMUNICATE_HEADER to definitive size.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>

Signed-off-by: Kun Qin <kuqin12@gmail.com>
---
 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c   | 13 ++++++++++++-
 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf |  1 +
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
index 599a0cd01d80..9508715fda24 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -34,6 +34,7 @@
 #include <Library/UefiRuntimeLib.h>
 #include <Library/PcdLib.h>
 #include <Library/ReportStatusCodeLib.h>
+#include <Library/SafeIntLib.h> // BZ3398
 
 #include "PiSmmCorePrivateData.h"
 
@@ -515,6 +516,7 @@ SmmCommunicationCommunicate (
   EFI_STATUS                  Status;
   EFI_SMM_COMMUNICATE_HEADER  *CommunicateHeader;
   BOOLEAN                     OldInSmm;
+  UINT64                      BZ3398_LongCommSize;
   UINTN                       TempCommSize;
 
   //
@@ -527,7 +529,16 @@ SmmCommunicationCommunicate (
   CommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER *) CommBuffer;
 
   if (CommSize == NULL) {
-    TempCommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + CommunicateHeader->MessageLength;
+    // BZ3398 Starts: Make MessageLength the same size in EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
+    Status = SafeUint64Add (OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data), CommunicateHeader->MessageLength, &BZ3398_LongCommSize);
+    if (EFI_ERROR (Status)) {
+      return EFI_INVALID_PARAMETER;
+    }
+    Status = SafeUint64ToUintn (BZ3398_LongCommSize, &TempCommSize);
+    if (EFI_ERROR (Status)) {
+      return EFI_INVALID_PARAMETER;
+    }
+    // BZ3398 Ends
   } else {
     TempCommSize = *CommSize;
     //
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
index 6109d6b5449c..87142e27fa47 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
@@ -46,6 +46,7 @@ [LibraryClasses]
   DxeServicesLib
   PcdLib
   ReportStatusCodeLib
+  SafeIntLib  #BZ3398
 
 [Protocols]
   gEfiSmmBase2ProtocolGuid                      ## PRODUCES
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76305): https://edk2.groups.io/g/devel/message/76305
Mute This Topic: https://groups.io/mt/83435923/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 2/5] MdeModulePkg: PiSmmIpl: Update MessageLength calculation for MmCommunicate
Posted by Wu, Hao A 4 years, 8 months ago 
A couple of minor comments below:


> -----Original Message-----
> From: Kun Qin <kuqin12@gmail.com>
> Sent: Thursday, June 10, 2021 9:43 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [PATCH v1 2/5] MdeModulePkg: PiSmmIpl: Update MessageLength
> calculation for MmCommunicate
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3398
> 
> This change updated calculation routine for MM communication in PiSmmIpl.
> It removes ambiguity brought in by UINTN variables from this routine and
> paves way for updating definition of field MessageLength in
> EFI_MM_COMMUNICATE_HEADER to definitive size.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> 
> Signed-off-by: Kun Qin <kuqin12@gmail.com>
> ---
>  MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c   | 13 ++++++++++++-
>  MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf |  1 +
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> index 599a0cd01d80..9508715fda24 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> @@ -34,6 +34,7 @@
>  #include <Library/UefiRuntimeLib.h>
>  #include <Library/PcdLib.h>
>  #include <Library/ReportStatusCodeLib.h>
> +#include <Library/SafeIntLib.h> // BZ3398


I suggest to remove the comment "// BZ3398" here.

I think users can use the 'blame' feature of the version control systems
together with the commit log message to find out the information.


> 
>  #include "PiSmmCorePrivateData.h"
> 
> @@ -515,6 +516,7 @@ SmmCommunicationCommunicate (
>    EFI_STATUS                  Status;
>    EFI_SMM_COMMUNICATE_HEADER  *CommunicateHeader;
>    BOOLEAN                     OldInSmm;
> +  UINT64                      BZ3398_LongCommSize;


Suggest to drop the "BZ3398_" prefix for the variable name.


>    UINTN                       TempCommSize;
> 
>    //
> @@ -527,7 +529,16 @@ SmmCommunicationCommunicate (
>    CommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER *)
> CommBuffer;
> 
>    if (CommSize == NULL) {
> -    TempCommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data)
> + CommunicateHeader->MessageLength;
> +    // BZ3398 Starts: Make MessageLength the same size in
> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.


Suggest to drop the "// BZ3398 Starts:" and "// BZ3398 Ends" comments pair here.

With above handled:
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> +    Status = SafeUint64Add (OFFSET_OF
> (EFI_SMM_COMMUNICATE_HEADER, Data), CommunicateHeader-
> >MessageLength, &BZ3398_LongCommSize);
> +    if (EFI_ERROR (Status)) {
> +      return EFI_INVALID_PARAMETER;
> +    }
> +    Status = SafeUint64ToUintn (BZ3398_LongCommSize, &TempCommSize);
> +    if (EFI_ERROR (Status)) {
> +      return EFI_INVALID_PARAMETER;
> +    }
> +    // BZ3398 Ends
>    } else {
>      TempCommSize = *CommSize;
>      //
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
> b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
> index 6109d6b5449c..87142e27fa47 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
> @@ -46,6 +46,7 @@ [LibraryClasses]
>    DxeServicesLib
>    PcdLib
>    ReportStatusCodeLib
> +  SafeIntLib  #BZ3398
> 
>  [Protocols]
>    gEfiSmmBase2ProtocolGuid                      ## PRODUCES
> --
> 2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76373): https://edk2.groups.io/g/devel/message/76373
Mute This Topic: https://groups.io/mt/83435923/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.