From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f175.google.com (mail-pl1-f175.google.com [209.85.214.175]) by mx.groups.io with SMTP id smtpd.web09.744.1640050720580338260 for ; Mon, 20 Dec 2021 17:38:40 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20210112 header.b=fYMNrMRn; spf=pass (domain: gmail.com, ip: 209.85.214.175, mailfrom: kuqin12@gmail.com) Received: by mail-pl1-f175.google.com with SMTP id p18so9438644pld.13 for ; Mon, 20 Dec 2021 17:38:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=message-id:date:mime-version:user-agent:subject:content-language:to :cc:references:from:in-reply-to:content-transfer-encoding; bh=x2S+Oiy5unZqRR1g0IuKFnRwkyZPwwOGGYd9Asnbekw=; b=fYMNrMRniZ6W6U+cIErQn0+7DXQFwbiS+41BxkgeI/T1o17wa4FlH7YkXboN0eg/6z rSlWLPPjAOrEx7biyI2MAjN8Uupv5dq0E1zulA+KsWKwNWuhIrLffdHqSlk9gchk2Wzx R4sOBb1hC/vW8qIUqUtNw860ZoOjgSAqhhWuq+f1mubUbWSedIlERrCJRCUVwjBgCLq9 B3v3SlmCepabBy3a9RupCBrrLAJekStbUh7Yut4DzgzZ1fUcrIDrD3x88bIj9kF5eE4v 7JZBWqUjq1DZ6LJUUjmz1uTGsPRclw5IqgCWEP8oWXuV7jhHWnP3TYhDFSNzvpny+UQ8 6/ZA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=x2S+Oiy5unZqRR1g0IuKFnRwkyZPwwOGGYd9Asnbekw=; b=P6/XGgErnRc9hcZExRf4AEGZO/FvZxh0dgf/4UZL84b7gDERHxf/dvvOIgeZkBjucE ykxt6PjWK5OTrsY9IAxxu8MwhPNDlwUy229UuHk4mOi+D/JRIwBDmjQBh5iuyR9lO748 6hXlMLSlgYH7sD4DHWmvDRfnZdKq22SyGn/nIC/XRWX9o9w+1ye/+Vv4oXZ+KUITXUIZ JnUPsbNq8GVeqXySwsp9GKhiHMCy8m4Wf5qKff/J0Vg8h9Hthw2HpiSYCZMIBLCfnBzW pngNZ4M4Ar8GvFEhln8wgENh/G3EnjB/alXk/ohJchUxLnfWCV9SrIzMptw2foAvvePZ Oqog== X-Gm-Message-State: AOAM533In9PmrR7IanmVeiwlDUeLdNxTyU8e5CvaFQEWlXmEdbCAeR4k 21Abrh9UJOiLotpdsaeUGvE= X-Google-Smtp-Source: ABdhPJxiFWA77ApuY0BfcuP8N7rzbNazcwifh3apZ3AerTU02vue5fC8czVAqFA7wWJAhz29vmpPCw== X-Received: by 2002:a17:90a:4894:: with SMTP id b20mr1158313pjh.207.1640050720192; Mon, 20 Dec 2021 17:38:40 -0800 (PST) Return-Path: Received: from [192.168.1.18] ([50.35.74.198]) by smtp.gmail.com with ESMTPSA id w184sm8948291pfw.218.2021.12.20.17.38.39 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 20 Dec 2021 17:38:39 -0800 (PST) Message-ID: <648c4ffd-cb86-cd7a-da05-26fdedd03f4d@gmail.com> Date: Mon, 20 Dec 2021 17:38:39 -0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 Subject: Re: [edk2-devel] [PATCH v1 2/2] ArmPkg: MmCommunicationDxe: Update MM communicate input arguments checks To: Sami Mujawar , devel@edk2.groups.io Cc: Leif Lindholm , Ard Biesheuvel , Bret Barkelew , Michael Kubacki , nd References: <20211130003902.1884-1-kuqin12@gmail.com> <20211130003902.1884-3-kuqin12@gmail.com> <486bca9b-c3ed-265c-76a6-5f8392f554c1@arm.com> From: "Kun Qin" In-Reply-To: <486bca9b-c3ed-265c-76a6-5f8392f554c1@arm.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 > > 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 >> Cc: Ard Biesheuvel >> Cc: Bret Barkelew >> Cc: Michael Kubacki >> >> Signed-off-by: Kun Qin >> --- >>   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 >