Re: [edk2-devel] [PATCH v1 3/5] MdeModulePkg: MemoryProfileInfo: Updated MessageLength calculation

[Kun Qin]

Hi Hao,

Thanks for pointing out the missing place. Will update this accordingly.
This patch series needs a PI spec update, I thought I should mark all 
changes with BZ#### before the spec update is taken. But I can drop them 
for the next patch version.

Regards,
Kun

On 06/11/2021 00:46, Wu, Hao A wrote:
-----Original Message-----
From: Kun Qin <kuqi...@gmail.com>
Sent: Thursday, June 10, 2021 9:43 AM
To: devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>
Subject: [PATCH v1 3/5] MdeModulePkg: MemoryProfileInfo: Updated
MessageLength calculation

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3398

This change replaced the calculation of communication buffer size from
explicitly adding the size of each member with the OFFSET macro function.
This will make the structure field defition change transparent to consumers.


I think there is one missing place in function GetSmramProfileData():

   MinimalSizeNeeded = sizeof (EFI_GUID) +
                       sizeof (UINTN) +
                       MAX (sizeof (SMRAM_PROFILE_PARAMETER_GET_PROFILE_INFO),
                            MAX (sizeof 
(SMRAM_PROFILE_PARAMETER_GET_PROFILE_DATA_BY_OFFSET),
                                 sizeof 
(SMRAM_PROFILE_PARAMETER_RECORDING_STATE)));

More inline comments below:



Cc: Jian J Wang <jian.j.w...@intel.com>
Cc: Hao A Wu <hao.a...@intel.com>

Signed-off-by: Kun Qin <kuqi...@gmail.com>
---
  MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c | 20
+++++++++++++++-----
  1 file changed, 15 insertions(+), 5 deletions(-)

diff --git
a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
index 191c31068545..39ed8b2e0484 100644
--- a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
+++
b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
@@ -1190,7 +1190,9 @@ GetSmramProfileData (
    CommRecordingState->Header.ReturnStatus = (UINT64)-1;
    CommRecordingState->RecordingState      =
MEMORY_PROFILE_RECORDING_DISABLE;

-  CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
MessageLength;
+  // BZ3398: Make MessageLength the same size in
EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
+  // The CommHeader->MessageLength contains a definitive value, thus
UINTN cast is safe here.


Please help to drop the explicit mention of BZ3398 in the comment.
How about using:
   //
   // The CommHeader->MessageLength contains a definitive value, thus UINTN 
cast is safe here.
   //

There are 4 more similar cases below.

Best Regards,
Hao Wu


+  CommSize = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) +
+ (UINTN)CommHeader->MessageLength;
    Status = SmmCommunication->Communicate (SmmCommunication,
CommBuffer, &CommSize);
    if (EFI_ERROR (Status)) {
      DEBUG ((EFI_D_ERROR, "SmramProfile: SmmCommunication - %r\n",
Status)); @@ -1213,7 +1215,9 @@ GetSmramProfileData (
      CommRecordingState->Header.ReturnStatus = (UINT64)-1;
      CommRecordingState->RecordingState      =
MEMORY_PROFILE_RECORDING_DISABLE;

-    CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
MessageLength;
+    // BZ3398: Make MessageLength the same size in
EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
+    // The CommHeader->MessageLength contains a definitive value, thus
UINTN cast is safe here.
+    CommSize = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) +
+ (UINTN)CommHeader->MessageLength;
      SmmCommunication->Communicate (SmmCommunication, CommBuffer,
&CommSize);
    }

@@ -1230,7 +1234,9 @@ GetSmramProfileData (
    CommGetProfileInfo->Header.ReturnStatus = (UINT64)-1;
    CommGetProfileInfo->ProfileSize         = 0;

-  CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
MessageLength;
+  // BZ3398: Make MessageLength the same size in
EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
+  // The CommHeader->MessageLength contains a definitive value, thus
UINTN cast is safe here.
+  CommSize = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) +
+ (UINTN)CommHeader->MessageLength;
    Status = SmmCommunication->Communicate (SmmCommunication,
CommBuffer, &CommSize);
    ASSERT_EFI_ERROR (Status);

@@ -1261,7 +1267,9 @@ GetSmramProfileData (
    CommGetProfileData->Header.DataLength   = sizeof
(*CommGetProfileData);
    CommGetProfileData->Header.ReturnStatus = (UINT64)-1;

-  CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
MessageLength;
+  // BZ3398: Make MessageLength the same size in
EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
+  // The CommHeader->MessageLength contains a definitive value, thus
UINTN cast is safe here.
+  CommSize = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) +
+ (UINTN)CommHeader->MessageLength;
    Buffer = (UINT8 *) CommHeader + CommSize;
    Size -= CommSize;

@@ -1320,7 +1328,9 @@ GetSmramProfileData (
      CommRecordingState->Header.ReturnStatus = (UINT64)-1;
      CommRecordingState->RecordingState      =
MEMORY_PROFILE_RECORDING_ENABLE;

-    CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
MessageLength;
+    // BZ3398: Make MessageLength the same size in
EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
+    // The CommHeader->MessageLength contains a definitive value, thus
UINTN cast is safe here.
+    CommSize = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) +
+ (UINTN)CommHeader->MessageLength;
      SmmCommunication->Communicate (SmmCommunication, CommBuffer,
&CommSize);
    }

--
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76412): https://edk2.groups.io/g/devel/message/76412
Mute This Topic: https://groups.io/mt/83435924/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-