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

["Ard Biesheuvel" <ardb@kernel.org>]

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
>