On Tue, 30 Nov 2021 at 01:39, Kun Qin <kuqi...@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 <l...@nuviainc.com>
> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>
> Cc: Bret Barkelew <bret.barke...@microsoft.com>
> Cc: Michael Kubacki <michael.kuba...@microsoft.com>
>
> Signed-off-by: Kun Qin <kuqi...@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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#84877): https://edk2.groups.io/g/devel/message/84877
Mute This Topic: https://groups.io/mt/87392597/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
