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

["Kun Qin" <kuqin12@gmail.com>]

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
>