On Tue, 30 Nov 2021 at 01:39, Kun Qin <kuqin12@...> wrote:

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

Current MM communicate routine from ArmPkg would conduct few steps before
proceeding with SMC calls. However, some inspection steps are different
from PI specification.

This patch updated MM communicate input argument inspection routine to
match the following PI descriptions:
1. Return code `EFI_INVALID_PARAMETER` represents "the `CommBuffer**`
parameters do not refer to the same location in memory".
2. `CommSize` represents "the size of the data buffer being passed in"
instead of "the size of the data being used from data buffer".
3. Regarding `MessageLength`, "if the `MessageLength` is zero, or too
large for the MM implementation to manage, the MM implementation must
update the `MessageLength` to reflect the size of the `Data` buffer that
it can tolerate".

A bullet list like this is usually a strong hint that the patch should
be split up, and this case is no different. Please split this up into
3 separate patches so that us poor reviewers have an easier job.

Cc: Leif Lindholm <leif@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Bret Barkelew <Bret.Barkelew@...>
Cc: Michael Kubacki <michael.kubacki@...>

Signed-off-by: Kun Qin <kuqin12@...>
---
ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c | 44 ++++++++++++--------
1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
index b1e309580988..8a2bd222957f 100644
--- a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
@@ -41,15 +41,19 @@ STATIC EFI_HANDLE mMmCommunicateHandle;

This function provides a service to send and receive messages from a registered UEFI service.

- @param[in] This The EFI_MM_COMMUNICATION_PROTOCOL instance.
- @param[in] CommBufferPhysical Physical address of the MM communication buffer
- @param[in] CommBufferVirtual Virtual address of the MM communication buffer
- @param[in] CommSize The size of the data buffer being passed in. On exit, the size of data
- being returned. Zero if the handler does not wish to reply with any data.
- This parameter is optional and may be NULL.
+ @param[in] This The EFI_MM_COMMUNICATION_PROTOCOL instance.
+ @param[in, out] CommBufferPhysical Physical address of the MM communication buffer
+ @param[in, out] CommBufferVirtual Virtual address of the MM communication buffer
+ @param[in, out] CommSize The size of the data buffer being passed in. On input, when not
+ omitted, the buffer should cover EFI_MM_COMMUNICATE_HEADER and the
+ value of MessageLength field. On exit, the size of data being
+ returned. Zero if the handler does not wish to reply with any data.
+ This parameter is optional and may be NULL.

@retval EFI_SUCCESS The message was successfully posted.
- @retval EFI_INVALID_PARAMETER CommBufferPhysical was NULL or CommBufferVirtual was NULL.
+ @retval EFI_INVALID_PARAMETER CommBufferPhysical or CommBufferVirtual was NULL, or integer value
+ pointed by CommSize does not cover EFI_MM_COMMUNICATE_HEADER and the
+ value of MessageLength field.
@retval EFI_BAD_BUFFER_SIZE The buffer is too large for the MM implementation.
If this error is returned, the MessageLength field
in the CommBuffer header or the integer pointed by
@@ -82,10 +86,11 @@ MmCommunication2Communicate (
//
// Check parameters
//
- if (CommBufferVirtual == NULL) {
+ if (CommBufferVirtual == NULL || CommBufferPhysical == NULL) {
return EFI_INVALID_PARAMETER;
}

+ Status = EFI_SUCCESS;
CommunicateHeader = CommBufferVirtual;
// CommBuffer is a mandatory parameter. Hence, Rely on
// MessageLength + Header to ascertain the
@@ -95,33 +100,38 @@ MmCommunication2Communicate (
sizeof (CommunicateHeader->HeaderGuid) +
sizeof (CommunicateHeader->MessageLength);

- // If the length of the CommBuffer is 0 then return the expected length.
- if (CommSize != 0) {
+ // If CommSize is not omitted, perform size inspection before proceeding.
+ if (CommSize != NULL) {
// This case can be used by the consumer of this driver to find out the
// max size that can be used for allocating CommBuffer.
if ((*CommSize == 0) ||
(*CommSize > mNsCommBuffMemRegion.Length)) {
*CommSize = mNsCommBuffMemRegion.Length;
- return EFI_BAD_BUFFER_SIZE;
+ Status = EFI_BAD_BUFFER_SIZE;
}
//
- // CommSize must match MessageLength + sizeof (EFI_MM_COMMUNICATE_HEADER);
+ // CommSize should cover at least MessageLength + sizeof (EFI_MM_COMMUNICATE_HEADER);
//
- if (*CommSize != BufferSize) {
- return EFI_INVALID_PARAMETER;
+ if (*CommSize < BufferSize) {
+ Status = EFI_INVALID_PARAMETER;
}
}

//
- // If the buffer size is 0 or greater than what can be tolerated by the MM
+ // If the message length is 0 or greater than what can be tolerated by the MM
// environment then return the expected size.
//
- if ((BufferSize == 0) ||
+ if ((CommunicateHeader->MessageLength == 0) ||
(BufferSize > mNsCommBuffMemRegion.Length)) {
CommunicateHeader->MessageLength = mNsCommBuffMemRegion.Length -
sizeof (CommunicateHeader->HeaderGuid) -
sizeof (CommunicateHeader->MessageLength);
- return EFI_BAD_BUFFER_SIZE;
+ Status = EFI_BAD_BUFFER_SIZE;
+ }
+
+ // MessageLength or CommSize check has failed, return here.
+ if (EFI_ERROR (Status)) {
+ return Status;
}

// SMC Function ID
--
2.32.0.windows.1