[PATCH v2 3/6] MdeModulePkg: MemoryProfileInfo: Updated MessageLength calculation


Wu, Hao A
 

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kun Qin
Sent: Friday, June 18, 2021 5:03 PM
To: devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
Subject: [edk2-devel] [PATCH v2 3/6] 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.

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

Signed-off-by: Kun Qin <kuqin12@gmail.com>
---

Notes:
v2:
- Added a missed case this change should cover [Hao]
- Removed "BZ" tags from comments [Hao]

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

Best Regards,
Hao Wu



MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c | 28
+++++++++++++++-----
1 file changed, 21 insertions(+), 7 deletions(-)

diff --git
a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
index 191c31068545..69f78c090e7c 100644
--- a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
+++
b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
@@ -1140,8 +1140,7 @@ GetSmramProfileData (
return Status;
}

- MinimalSizeNeeded = sizeof (EFI_GUID) +
- sizeof (UINTN) +
+ MinimalSizeNeeded = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER,
Data) +
MAX (sizeof
(SMRAM_PROFILE_PARAMETER_GET_PROFILE_INFO),
MAX (sizeof
(SMRAM_PROFILE_PARAMETER_GET_PROFILE_DATA_BY_OFFSET),
sizeof
(SMRAM_PROFILE_PARAMETER_RECORDING_STATE)));
@@ -1190,7 +1189,10 @@ GetSmramProfileData (
CommRecordingState->Header.ReturnStatus = (UINT64)-1;
CommRecordingState->RecordingState =
MEMORY_PROFILE_RECORDING_DISABLE;

- CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
MessageLength;
+ //
+ // 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);
if (EFI_ERROR (Status)) {
DEBUG ((EFI_D_ERROR, "SmramProfile: SmmCommunication - %r\n",
Status));
@@ -1213,7 +1215,10 @@ GetSmramProfileData (
CommRecordingState->Header.ReturnStatus = (UINT64)-1;
CommRecordingState->RecordingState =
MEMORY_PROFILE_RECORDING_DISABLE;

- CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
MessageLength;
+ //
+ // 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 +1235,10 @@ GetSmramProfileData (
CommGetProfileInfo->Header.ReturnStatus = (UINT64)-1;
CommGetProfileInfo->ProfileSize = 0;

- CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
MessageLength;
+ //
+ // 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 +1269,10 @@ GetSmramProfileData (
CommGetProfileData->Header.DataLength = sizeof
(*CommGetProfileData);
CommGetProfileData->Header.ReturnStatus = (UINT64)-1;

- CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
MessageLength;
+ //
+ // 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 +1331,10 @@ GetSmramProfileData (
CommRecordingState->Header.ReturnStatus = (UINT64)-1;
CommRecordingState->RecordingState =
MEMORY_PROFILE_RECORDING_ENABLE;

- CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
MessageLength;
+ //
+ // 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





Kun Qin
 

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.

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

Signed-off-by: Kun Qin <kuqin12@gmail.com>
---

Notes:
v2:
- Added a missed case this change should cover [Hao]
- Removed "BZ" tags from comments [Hao]

MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c | 28 +++++++++++++++-----
1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
index 191c31068545..69f78c090e7c 100644
--- a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
+++ b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
@@ -1140,8 +1140,7 @@ GetSmramProfileData (
return Status;
}

- MinimalSizeNeeded = sizeof (EFI_GUID) +
- sizeof (UINTN) +
+ MinimalSizeNeeded = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) +
MAX (sizeof (SMRAM_PROFILE_PARAMETER_GET_PROFILE_INFO),
MAX (sizeof (SMRAM_PROFILE_PARAMETER_GET_PROFILE_DATA_BY_OFFSET),
sizeof (SMRAM_PROFILE_PARAMETER_RECORDING_STATE)));
@@ -1190,7 +1189,10 @@ GetSmramProfileData (
CommRecordingState->Header.ReturnStatus = (UINT64)-1;
CommRecordingState->RecordingState = MEMORY_PROFILE_RECORDING_DISABLE;

- CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader->MessageLength;
+ //
+ // 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);
if (EFI_ERROR (Status)) {
DEBUG ((EFI_D_ERROR, "SmramProfile: SmmCommunication - %r\n", Status));
@@ -1213,7 +1215,10 @@ GetSmramProfileData (
CommRecordingState->Header.ReturnStatus = (UINT64)-1;
CommRecordingState->RecordingState = MEMORY_PROFILE_RECORDING_DISABLE;

- CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader->MessageLength;
+ //
+ // 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 +1235,10 @@ GetSmramProfileData (
CommGetProfileInfo->Header.ReturnStatus = (UINT64)-1;
CommGetProfileInfo->ProfileSize = 0;

- CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader->MessageLength;
+ //
+ // 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 +1269,10 @@ GetSmramProfileData (
CommGetProfileData->Header.DataLength = sizeof (*CommGetProfileData);
CommGetProfileData->Header.ReturnStatus = (UINT64)-1;

- CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader->MessageLength;
+ //
+ // 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 +1331,10 @@ GetSmramProfileData (
CommRecordingState->Header.ReturnStatus = (UINT64)-1;
CommRecordingState->RecordingState = MEMORY_PROFILE_RECORDING_ENABLE;

- CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader->MessageLength;
+ //
+ // 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