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

["Sami Mujawar" <sami.mujawar@arm.com>]

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