[PATCH v1 0/2] MM communicate functionality in variable policy

[PATCH v1 0/2] MM communicate functionality in variable policy

[Kun Qin]

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

Currently, setups with variable policy operations used together with MM
communicate from ArmPkg could fail with `EFI_INVALID_PARAMETER`. This was
due to the errors from 2 following aspects: 

1. For variable policy implementations in MdeModulePkg, the DXE runtime
agent would communicate to MM to disable, register or query policies.
However, during these operations, the MessageLength calculation is
including MM communicate header. This could lead to MM agent read data
across the given buffer boundary and/or trigger other errors.

2. On the other hand, current MM communicate routine from ArmPkg would
fail the function if the input message length does not equal to input
buffer size.

As defined in PI specification, the `CommSize`, when as input, should
stand for "The size of the data buffer being passed in", which would mean
the maximal number of bytes `CommBuffer` can hold. In turn, the value of
this input parameter can be used for MM handlers to determine whether the
output data is too large to fit in this buffer. Enforcing the incoming
buffer to hold exactly the number of used bytes mismatches with the PI
spec description.

This change fix MessageLength field calculation from variable policy and
updated input argument inspections from MM communicate routine in ArmPkg
to match PI spec descriptions.

Patch v1 branch: https://github.com/kuqin12/edk2/tree/mm_communicate_check

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Michael Kubacki <michael.kubacki@microsoft.com>

Kun Qin (2):
  MdeModulePkg: VariableSmmRuntimeDxe: Fix Variable Policy Message
    Length
  ArmPkg: MmCommunicationDxe: Update MM communicate input arguments
    checks

 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c               | 44 ++++++++++++--------
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c | 10 ++---
 2 files changed, 32 insertions(+), 22 deletions(-)

-- 
2.32.0.windows.1

[PATCH v1 1/2] MdeModulePkg: VariableSmmRuntimeDxe: Fix Variable Policy Message Length

[Kun Qin]

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

In EDKII implementation of variable policy, the DXE runtime agent would
communicate to MM to disable, register or query policies. However, these
operations populate the value of MessageLength that includes communicate
header to include MM communicate header, which mismatches with the
description of PI specification.

This fix will correct the MessageLength field calculation to exclude
the size of MM_COMMUNICATE_HEADER.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Michael Kubacki <michael.kubacki@microsoft.com>

Signed-off-by: Kun Qin <kuqin12@gmail.com>
---
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c
index 6ae69dffe025..486f95720c92 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c
@@ -88,7 +88,7 @@ ProtocolDisableVariablePolicy (
   CommHeader    = mMmCommunicationBuffer;
   PolicyHeader  = (VAR_CHECK_POLICY_COMM_HEADER*)&CommHeader->Data;
   CopyGuid( &CommHeader->HeaderGuid, &gVarCheckPolicyLibMmiHandlerGuid );
-  CommHeader->MessageLength = BufferSize;
+  CommHeader->MessageLength = BufferSize - OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data);
   PolicyHeader->Signature   = VAR_CHECK_POLICY_COMM_SIG;
   PolicyHeader->Revision    = VAR_CHECK_POLICY_COMM_REVISION;
   PolicyHeader->Command     = VAR_CHECK_POLICY_COMMAND_DISABLE;
@@ -138,7 +138,7 @@ ProtocolIsVariablePolicyEnabled (
   PolicyHeader  = (VAR_CHECK_POLICY_COMM_HEADER*)&CommHeader->Data;
   CommandParams = (VAR_CHECK_POLICY_COMM_IS_ENABLED_PARAMS*)(PolicyHeader + 1);
   CopyGuid( &CommHeader->HeaderGuid, &gVarCheckPolicyLibMmiHandlerGuid );
-  CommHeader->MessageLength = BufferSize;
+  CommHeader->MessageLength = BufferSize - OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data);
   PolicyHeader->Signature   = VAR_CHECK_POLICY_COMM_SIG;
   PolicyHeader->Revision    = VAR_CHECK_POLICY_COMM_REVISION;
   PolicyHeader->Command     = VAR_CHECK_POLICY_COMMAND_IS_ENABLED;
@@ -208,7 +208,7 @@ ProtocolRegisterVariablePolicy (
   PolicyHeader  = (VAR_CHECK_POLICY_COMM_HEADER*)&CommHeader->Data;
   PolicyBuffer  = (VOID*)(PolicyHeader + 1);
   CopyGuid( &CommHeader->HeaderGuid, &gVarCheckPolicyLibMmiHandlerGuid );
-  CommHeader->MessageLength = BufferSize;
+  CommHeader->MessageLength = BufferSize - OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data);
   PolicyHeader->Signature   = VAR_CHECK_POLICY_COMM_SIG;
   PolicyHeader->Revision    = VAR_CHECK_POLICY_COMM_REVISION;
   PolicyHeader->Command     = VAR_CHECK_POLICY_COMMAND_REGISTER;
@@ -266,7 +266,7 @@ DumpVariablePolicyHelper (
   PolicyHeader  = (VAR_CHECK_POLICY_COMM_HEADER*)&CommHeader->Data;
   CommandParams = (VAR_CHECK_POLICY_COMM_DUMP_PARAMS*)(PolicyHeader + 1);
   CopyGuid( &CommHeader->HeaderGuid, &gVarCheckPolicyLibMmiHandlerGuid );
-  CommHeader->MessageLength = BufferSize;
+  CommHeader->MessageLength = BufferSize - OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data);
   PolicyHeader->Signature   = VAR_CHECK_POLICY_COMM_SIG;
   PolicyHeader->Revision    = VAR_CHECK_POLICY_COMM_REVISION;
   PolicyHeader->Command     = VAR_CHECK_POLICY_COMMAND_DUMP;
@@ -395,7 +395,7 @@ ProtocolLockVariablePolicy (
   CommHeader    = mMmCommunicationBuffer;
   PolicyHeader  = (VAR_CHECK_POLICY_COMM_HEADER*)&CommHeader->Data;
   CopyGuid( &CommHeader->HeaderGuid, &gVarCheckPolicyLibMmiHandlerGuid );
-  CommHeader->MessageLength = BufferSize;
+  CommHeader->MessageLength = BufferSize - OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data);
   PolicyHeader->Signature   = VAR_CHECK_POLICY_COMM_SIG;
   PolicyHeader->Revision    = VAR_CHECK_POLICY_COMM_REVISION;
   PolicyHeader->Command     = VAR_CHECK_POLICY_COMMAND_LOCK;
-- 
2.32.0.windows.1

[PATCH v1 2/2] ArmPkg: MmCommunicationDxe: Update MM communicate input arguments checks

[Kun Qin]

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".

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

Signed-off-by: Kun Qin <kuqin12@gmail.com>
---
 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

Re: [edk2-devel] [PATCH v1 2/2] ArmPkg: MmCommunicationDxe: Update MM communicate input arguments checks

[Sami Mujawar]

Hi Kun,

Thank you for this patch.

These changes look good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar


On 30/11/2021 12:39 AM, Kun Qin via groups.io 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".
>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> Cc: Michael Kubacki <michael.kubacki@microsoft.com>
>
> Signed-off-by: Kun Qin <kuqin12@gmail.com>
> ---
>   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

Re: [edk2-devel] [PATCH v1 2/2] ArmPkg: MmCommunicationDxe: Update MM communicate input arguments checks

[Kun Qin]

Hi Sami,

Thanks for your review. But this v1 patch was splitted into multiple 
patches as in https://edk2.groups.io/g/devel/message/85116. Please feel 
free leave feedback on the new series.

Regards,
Kun

On 12/13/2021 13:03, Sami Mujawar wrote:
> Hi Kun,
> 
> Thank you for this patch.
> 
> These changes look good to me.
> 
> Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
> 
> Regards,
> 
> Sami Mujawar
> 
> 
> On 30/11/2021 12:39 AM, Kun Qin via groups.io 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".
>>
>> Cc: Leif Lindholm <leif@nuviainc.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
>> Cc: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>> ---
>>   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
> 

Re: [PATCH v1 2/2] ArmPkg: MmCommunicationDxe: Update MM communicate input arguments checks

[Ard Biesheuvel]

On Tue, 30 Nov 2021 at 01:39, Kun Qin <kuqin12@gmail.com> 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@nuviainc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> Cc: Michael Kubacki <michael.kubacki@microsoft.com>
>
> Signed-off-by: Kun Qin <kuqin12@gmail.com>
> ---
>  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
>

Re: [PATCH v1 2/2] ArmPkg: MmCommunicationDxe: Update MM communicate input arguments checks

[Kun Qin]

Sure, the updated patches are sent here: 
https://edk2.groups.io/g/devel/message/85116. Please provide any further 
feedback. Any input is appreciated.

Regards,
Kun

On 12/15/2021 00:52, Ard Biesheuvel wrote:
> On Tue, 30 Nov 2021 at 01:39, Kun Qin <kuqin12@gmail.com> 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@nuviainc.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
>> Cc: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>> ---
>>   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
>>