public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v3 0/6] MM communicate functionality in variable policy
@ 2022-01-25 19:39 Kun Qin
  2022-01-25 19:39 ` [PATCH v3 1/6] MdeModulePkg: VariableSmmRuntimeDxe: Fix Variable Policy Message Length Kun Qin
                   ` (6 more replies)
  0 siblings, 7 replies; 9+ messages in thread
From: Kun Qin @ 2022-01-25 19:39 UTC (permalink / raw)
  To: devel
  Cc: Jian J Wang, Liming Gao, Hao A Wu, Michael D Kinney, Zhiguang Liu,
	Leif Lindholm, Ard Biesheuvel, Bret Barkelew, Michael Kubacki,
	Sami Mujawar

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3709
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3751

This patch series is a follow up of previous submission:
https://edk2.groups.io/g/devel/message/85116

All v3 patches added "Reviewed-By" tags collected during v2 series.

Patch v3 branch: https://github.com/kuqin12/edk2/tree/mm_communicate_check_v3

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
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>
Cc: Sami Mujawar <sami.mujawar@arm.com>

Kun Qin (6):
  MdeModulePkg: VariableSmmRuntimeDxe: Fix Variable Policy Message
    Length
  MdePkg: MmCommunication2: Update MM communicate2 function description
  ArmPkg: MmCommunicationDxe: MM communicate function argument
    attributes
  ArmPkg: MmCommunicationDxe: Update MM communicate `CommBuffer**`
    checks
  ArmPkg: MmCommunicationDxe: Update MM communicate `CommSize` check
  ArmPkg: MmCommunicationDxe: Update MM communicate `MessageLength`
    check

 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c               | 46 ++++++++++++--------
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c | 10 ++---
 MdePkg/Include/Protocol/MmCommunication2.h                        | 13 +++---
 3 files changed, 41 insertions(+), 28 deletions(-)

-- 
2.34.1.windows.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 1/6] MdeModulePkg: VariableSmmRuntimeDxe: Fix Variable Policy Message Length
  2022-01-25 19:39 [PATCH v3 0/6] MM communicate functionality in variable policy Kun Qin
@ 2022-01-25 19:39 ` Kun Qin
  2022-01-25 19:39 ` [PATCH v3 2/6] MdePkg: MmCommunication2: Update MM communicate2 function description Kun Qin
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 9+ messages in thread
From: Kun Qin @ 2022-01-25 19:39 UTC (permalink / raw)
  To: devel; +Cc: Jian J Wang, Liming Gao, Hao A Wu, Bret Barkelew, Michael Kubacki

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3709

In EDKII implementation of variable policy, the DXE runtime agent would
communicate to MM to disable, register or query policies. However, these
operations populate the value of MessageLength that includes communicate
header to include MM communicate header, which mismatches with the
description of PI specification.

This fix will correct the MessageLength field calculation to exclude
the size of MM_COMMUNICATE_HEADER.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Michael Kubacki <michael.kubacki@microsoft.com>

Signed-off-by: Kun Qin <kuqin12@gmail.com>
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
---

Notes:
    v2:
    - No review, no updates
    
    v3:
    - Added reviewed-by tag [Liming]

 MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c
index 672a2293bcb1..b2094fbcd6ea 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c
@@ -89,7 +89,7 @@ ProtocolDisableVariablePolicy (
   CommHeader   = mMmCommunicationBuffer;
   PolicyHeader = (VAR_CHECK_POLICY_COMM_HEADER *)&CommHeader->Data;
   CopyGuid (&CommHeader->HeaderGuid, &gVarCheckPolicyLibMmiHandlerGuid);
-  CommHeader->MessageLength = BufferSize;
+  CommHeader->MessageLength = BufferSize - OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data);
   PolicyHeader->Signature   = VAR_CHECK_POLICY_COMM_SIG;
   PolicyHeader->Revision    = VAR_CHECK_POLICY_COMM_REVISION;
   PolicyHeader->Command     = VAR_CHECK_POLICY_COMMAND_DISABLE;
@@ -138,7 +138,7 @@ ProtocolIsVariablePolicyEnabled (
   PolicyHeader  = (VAR_CHECK_POLICY_COMM_HEADER *)&CommHeader->Data;
   CommandParams = (VAR_CHECK_POLICY_COMM_IS_ENABLED_PARAMS *)(PolicyHeader + 1);
   CopyGuid (&CommHeader->HeaderGuid, &gVarCheckPolicyLibMmiHandlerGuid);
-  CommHeader->MessageLength = BufferSize;
+  CommHeader->MessageLength = BufferSize - OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data);
   PolicyHeader->Signature   = VAR_CHECK_POLICY_COMM_SIG;
   PolicyHeader->Revision    = VAR_CHECK_POLICY_COMM_REVISION;
   PolicyHeader->Command     = VAR_CHECK_POLICY_COMMAND_IS_ENABLED;
@@ -213,7 +213,7 @@ ProtocolRegisterVariablePolicy (
   PolicyHeader = (VAR_CHECK_POLICY_COMM_HEADER *)&CommHeader->Data;
   PolicyBuffer = (VOID *)(PolicyHeader + 1);
   CopyGuid (&CommHeader->HeaderGuid, &gVarCheckPolicyLibMmiHandlerGuid);
-  CommHeader->MessageLength = BufferSize;
+  CommHeader->MessageLength = BufferSize - OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data);
   PolicyHeader->Signature   = VAR_CHECK_POLICY_COMM_SIG;
   PolicyHeader->Revision    = VAR_CHECK_POLICY_COMM_REVISION;
   PolicyHeader->Command     = VAR_CHECK_POLICY_COMMAND_REGISTER;
@@ -270,7 +270,7 @@ DumpVariablePolicyHelper (
   PolicyHeader  = (VAR_CHECK_POLICY_COMM_HEADER *)&CommHeader->Data;
   CommandParams = (VAR_CHECK_POLICY_COMM_DUMP_PARAMS *)(PolicyHeader + 1);
   CopyGuid (&CommHeader->HeaderGuid, &gVarCheckPolicyLibMmiHandlerGuid);
-  CommHeader->MessageLength = BufferSize;
+  CommHeader->MessageLength = BufferSize - OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data);
   PolicyHeader->Signature   = VAR_CHECK_POLICY_COMM_SIG;
   PolicyHeader->Revision    = VAR_CHECK_POLICY_COMM_REVISION;
   PolicyHeader->Command     = VAR_CHECK_POLICY_COMMAND_DUMP;
@@ -397,7 +397,7 @@ ProtocolLockVariablePolicy (
   CommHeader   = mMmCommunicationBuffer;
   PolicyHeader = (VAR_CHECK_POLICY_COMM_HEADER *)&CommHeader->Data;
   CopyGuid (&CommHeader->HeaderGuid, &gVarCheckPolicyLibMmiHandlerGuid);
-  CommHeader->MessageLength = BufferSize;
+  CommHeader->MessageLength = BufferSize - OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data);
   PolicyHeader->Signature   = VAR_CHECK_POLICY_COMM_SIG;
   PolicyHeader->Revision    = VAR_CHECK_POLICY_COMM_REVISION;
   PolicyHeader->Command     = VAR_CHECK_POLICY_COMMAND_LOCK;
-- 
2.34.1.windows.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 2/6] MdePkg: MmCommunication2: Update MM communicate2 function description
  2022-01-25 19:39 [PATCH v3 0/6] MM communicate functionality in variable policy Kun Qin
  2022-01-25 19:39 ` [PATCH v3 1/6] MdeModulePkg: VariableSmmRuntimeDxe: Fix Variable Policy Message Length Kun Qin
@ 2022-01-25 19:39 ` Kun Qin
  2022-01-25 19:39 ` [PATCH v3 3/6] ArmPkg: MmCommunicationDxe: MM communicate function argument attributes Kun Qin
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 9+ messages in thread
From: Kun Qin @ 2022-01-25 19:39 UTC (permalink / raw)
  To: devel; +Cc: Michael D Kinney, Liming Gao, Zhiguang Liu

Current MM communicate2 function definition described input arguments
`CommBufferPhysical`, `CommBufferVirtual` and `CommSize` as input only,
which mismatches with the "input and output type" as in PI specification.

This change updated function descriptions of MM communite2 definition to
match input argument types.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>

Signed-off-by: Kun Qin <kuqin12@gmail.com>
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
---

Notes:
    v2:
    - Newly added
    
    v3:
    - Added reviewed-by tag [Liming]

 MdePkg/Include/Protocol/MmCommunication2.h | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/MdePkg/Include/Protocol/MmCommunication2.h b/MdePkg/Include/Protocol/MmCommunication2.h
index 3495a7327f76..1b56320c7fff 100644
--- a/MdePkg/Include/Protocol/MmCommunication2.h
+++ b/MdePkg/Include/Protocol/MmCommunication2.h
@@ -27,12 +27,13 @@ typedef struct _EFI_MM_COMMUNICATION2_PROTOCOL EFI_MM_COMMUNICATION2_PROTOCOL;
 
   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 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.
-- 
2.34.1.windows.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 3/6] ArmPkg: MmCommunicationDxe: MM communicate function argument attributes
  2022-01-25 19:39 [PATCH v3 0/6] MM communicate functionality in variable policy Kun Qin
  2022-01-25 19:39 ` [PATCH v3 1/6] MdeModulePkg: VariableSmmRuntimeDxe: Fix Variable Policy Message Length Kun Qin
  2022-01-25 19:39 ` [PATCH v3 2/6] MdePkg: MmCommunication2: Update MM communicate2 function description Kun Qin
@ 2022-01-25 19:39 ` Kun Qin
  2022-01-25 19:39 ` [PATCH v3 4/6] ArmPkg: MmCommunicationDxe: Update MM communicate `CommBuffer**` checks Kun Qin
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 9+ messages in thread
From: Kun Qin @ 2022-01-25 19:39 UTC (permalink / raw)
  To: devel
  Cc: Leif Lindholm, Ard Biesheuvel, Bret Barkelew, Michael Kubacki,
	Sami Mujawar

Current MM communicate2 function from ArmPkg described input arguments
`CommBufferPhysical`, `CommBufferVirtual` and `CommSize` as input only,
which mismatches with the "input and output type" as in PI specification.

This change updated function descriptions of MM communite2 to match input
argument types.

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>
Cc: Sami Mujawar <sami.mujawar@arm.com>

Signed-off-by: Kun Qin <kuqin12@gmail.com>
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
---

Notes:
    v2:
    - Splitting patch into 1 of 4 [Ard]
    
    v3:
    - Added cc entry and reviewed-by tag [Sami]

 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
index 7c8284104d87..7f756a32d4e0 100644
--- a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
@@ -41,12 +41,13 @@ 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 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.
-- 
2.34.1.windows.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 4/6] ArmPkg: MmCommunicationDxe: Update MM communicate `CommBuffer**` checks
  2022-01-25 19:39 [PATCH v3 0/6] MM communicate functionality in variable policy Kun Qin
                   ` (2 preceding siblings ...)
  2022-01-25 19:39 ` [PATCH v3 3/6] ArmPkg: MmCommunicationDxe: MM communicate function argument attributes Kun Qin
@ 2022-01-25 19:39 ` Kun Qin
  2022-01-25 19:39 ` [PATCH v3 5/6] ArmPkg: MmCommunicationDxe: Update MM communicate `CommSize` check Kun Qin
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 9+ messages in thread
From: Kun Qin @ 2022-01-25 19:39 UTC (permalink / raw)
  To: devel
  Cc: Leif Lindholm, Ard Biesheuvel, Bret Barkelew, Michael Kubacki,
	Sami Mujawar

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3751

Current MM communicate routine from ArmPkg would conduct few checks prior
to proceeding with SMC calls. However, the inspection step is different
from PI specification.

This patch updated MM communicate input argument inspection routine to
assure that return code `EFI_INVALID_PARAMETER` represents "the
`CommBuffer**` parameters do not refer to the same location in memory",
as described by `EFI_MM_COMMUNICATION2_PROTOCOL.Communicate()` section
in PI specification.

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>
Cc: Sami Mujawar <sami.mujawar@arm.com>

Signed-off-by: Kun Qin <kuqin12@gmail.com>
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
---

Notes:
    v2:
    - Splitting patch into 2 of 4 [Ard]
    - Uncrustify style update
    
    v3:
    - Added cc entry and reviewed-by tag [Sami]

 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
index 7f756a32d4e0..0283be430dff 100644
--- a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
@@ -83,7 +83,7 @@ MmCommunication2Communicate (
   //
   // Check parameters
   //
-  if (CommBufferVirtual == NULL) {
+  if ((CommBufferVirtual == NULL) || (CommBufferPhysical == NULL)) {
     return EFI_INVALID_PARAMETER;
   }
 
-- 
2.34.1.windows.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 5/6] ArmPkg: MmCommunicationDxe: Update MM communicate `CommSize` check
  2022-01-25 19:39 [PATCH v3 0/6] MM communicate functionality in variable policy Kun Qin
                   ` (3 preceding siblings ...)
  2022-01-25 19:39 ` [PATCH v3 4/6] ArmPkg: MmCommunicationDxe: Update MM communicate `CommBuffer**` checks Kun Qin
@ 2022-01-25 19:39 ` Kun Qin
  2022-01-25 19:39 ` [PATCH v3 6/6] ArmPkg: MmCommunicationDxe: Update MM communicate `MessageLength` check Kun Qin
  2022-01-26  1:41 ` 回复: [PATCH v3 0/6] MM communicate functionality in variable policy gaoliming
  6 siblings, 0 replies; 9+ messages in thread
From: Kun Qin @ 2022-01-25 19:39 UTC (permalink / raw)
  To: devel
  Cc: Leif Lindholm, Ard Biesheuvel, Bret Barkelew, Michael Kubacki,
	Sami Mujawar

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3751

Current MM communicate routine from ArmPkg would conduct few checks prior
to proceeding with SMC calls. However, the inspection step is different
from PI specification.

This patch updated MM communicate input argument inspection routine to
assure `CommSize` represents "the size of the data buffer being passed
in" instead of the size of the data being used from data buffer, as
described by section `EFI_MM_COMMUNICATION2_PROTOCOL.Communicate()` in PI
specification.

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>
Cc: Sami Mujawar <sami.mujawar@arm.com>

Signed-off-by: Kun Qin <kuqin12@gmail.com>
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
---

Notes:
    v2:
    - Splitting patch into 3 of 4 [Ard]
    
    v3:
    - Added cc entry and reviewed-by tag [Sami]

 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
index 0283be430dff..2f89b7c5b6c4 100644
--- a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
@@ -44,13 +44,18 @@ STATIC EFI_HANDLE  mMmCommunicateHandle;
   @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 exit, the
-                                      size of data being returned. Zero if the handler does not
+  @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
@@ -96,8 +101,8 @@ 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) ||
@@ -108,9 +113,9 @@ MmCommunication2Communicate (
     }
 
     //
-    // CommSize must match MessageLength + sizeof (EFI_MM_COMMUNICATE_HEADER);
+    // CommSize should cover at least MessageLength + sizeof (EFI_MM_COMMUNICATE_HEADER);
     //
-    if (*CommSize != BufferSize) {
+    if (*CommSize < BufferSize) {
       return EFI_INVALID_PARAMETER;
     }
   }
-- 
2.34.1.windows.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 6/6] ArmPkg: MmCommunicationDxe: Update MM communicate `MessageLength` check
  2022-01-25 19:39 [PATCH v3 0/6] MM communicate functionality in variable policy Kun Qin
                   ` (4 preceding siblings ...)
  2022-01-25 19:39 ` [PATCH v3 5/6] ArmPkg: MmCommunicationDxe: Update MM communicate `CommSize` check Kun Qin
@ 2022-01-25 19:39 ` Kun Qin
  2022-01-26  1:41 ` 回复: [PATCH v3 0/6] MM communicate functionality in variable policy gaoliming
  6 siblings, 0 replies; 9+ messages in thread
From: Kun Qin @ 2022-01-25 19:39 UTC (permalink / raw)
  To: devel
  Cc: Leif Lindholm, Ard Biesheuvel, Bret Barkelew, Michael Kubacki,
	Sami Mujawar

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3751

Current MM communicate routine from ArmPkg would conduct few checks prior
to proceeding with SMC calls. However, the inspection step is different
from PI specification.

This patch updated MM communicate input argument inspection routine to
assure that "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", as described by `EFI_MM_COMMUNICATION_PROTOCOL.Communicate()`
section in PI specification.

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>
Cc: Sami Mujawar <sami.mujawar@arm.com>

Signed-off-by: Kun Qin <kuqin12@gmail.com>
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
---

Notes:
    v2:
    - Splitting patch into 4 of 4 [Ard]
    - Uncrustify style update
    
    v3:
    - Added cc entry and reviewed-by tag [Sami]

 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
index 2f89b7c5b6c4..85d9034555f0 100644
--- a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
@@ -92,6 +92,7 @@ MmCommunication2Communicate (
     return EFI_INVALID_PARAMETER;
   }
 
+  Status            = EFI_SUCCESS;
   CommunicateHeader = CommBufferVirtual;
   // CommBuffer is a mandatory parameter. Hence, Rely on
   // MessageLength + Header to ascertain the
@@ -109,28 +110,33 @@ MmCommunication2Communicate (
         (*CommSize > mNsCommBuffMemRegion.Length))
     {
       *CommSize = mNsCommBuffMemRegion.Length;
-      return EFI_BAD_BUFFER_SIZE;
+      Status    = EFI_BAD_BUFFER_SIZE;
     }
 
     //
     // CommSize should cover at least MessageLength + sizeof (EFI_MM_COMMUNICATE_HEADER);
     //
     if (*CommSize < BufferSize) {
-      return EFI_INVALID_PARAMETER;
+      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.34.1.windows.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* 回复: [PATCH v3 0/6] MM communicate functionality in variable policy
  2022-01-25 19:39 [PATCH v3 0/6] MM communicate functionality in variable policy Kun Qin
                   ` (5 preceding siblings ...)
  2022-01-25 19:39 ` [PATCH v3 6/6] ArmPkg: MmCommunicationDxe: Update MM communicate `MessageLength` check Kun Qin
@ 2022-01-26  1:41 ` gaoliming
  2022-01-27 23:05   ` [edk2-devel] " Kun Qin
  6 siblings, 1 reply; 9+ messages in thread
From: gaoliming @ 2022-01-26  1:41 UTC (permalink / raw)
  To: 'Kun Qin', devel
  Cc: 'Jian J Wang', 'Hao A Wu',
	'Michael D Kinney', 'Zhiguang Liu',
	'Leif Lindholm', 'Ard Biesheuvel',
	'Bret Barkelew', 'Michael Kubacki',
	'Sami Mujawar'

Hi, all
  If no other comment, I will merge this patch set tomorrow. 

Thanks
Liming
> -----邮件原件-----
> 发件人: Kun Qin <kuqin12@gmail.com>
> 发送时间: 2022年1月26日 3:39
> 收件人: devel@edk2.groups.io
> 抄送: Jian J Wang <jian.j.wang@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Hao A Wu <hao.a.wu@intel.com>; Michael D
> Kinney <michael.d.kinney@intel.com>; Zhiguang Liu
> <zhiguang.liu@intel.com>; Leif Lindholm <leif@nuviainc.com>; Ard
> Biesheuvel <ardb+tianocore@kernel.org>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Michael Kubacki
> <michael.kubacki@microsoft.com>; Sami Mujawar <sami.mujawar@arm.com>
> 主题: [PATCH v3 0/6] MM communicate functionality in variable policy
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3709
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3751
> 
> This patch series is a follow up of previous submission:
> https://edk2.groups.io/g/devel/message/85116
> 
> All v3 patches added "Reviewed-By" tags collected during v2 series.
> 
> Patch v3 branch:
> https://github.com/kuqin12/edk2/tree/mm_communicate_check_v3
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> 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>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> 
> Kun Qin (6):
>   MdeModulePkg: VariableSmmRuntimeDxe: Fix Variable Policy Message
>     Length
>   MdePkg: MmCommunication2: Update MM communicate2 function
> description
>   ArmPkg: MmCommunicationDxe: MM communicate function argument
>     attributes
>   ArmPkg: MmCommunicationDxe: Update MM communicate
> `CommBuffer**`
>     checks
>   ArmPkg: MmCommunicationDxe: Update MM communicate `CommSize`
> check
>   ArmPkg: MmCommunicationDxe: Update MM communicate
> `MessageLength`
>     check
> 
>  ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> | 46 ++++++++++++--------
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c |
> 10 ++---
>  MdePkg/Include/Protocol/MmCommunication2.h
> | 13 +++---
>  3 files changed, 41 insertions(+), 28 deletions(-)
> 
> --
> 2.34.1.windows.1




^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] 回复: [PATCH v3 0/6] MM communicate functionality in variable policy
  2022-01-26  1:41 ` 回复: [PATCH v3 0/6] MM communicate functionality in variable policy gaoliming
@ 2022-01-27 23:05   ` Kun Qin
  0 siblings, 0 replies; 9+ messages in thread
From: Kun Qin @ 2022-01-27 23:05 UTC (permalink / raw)
  To: devel, gaoliming

Hi Liming,

Thanks for the help!

Regards,
Kun

On 01/25/2022 17:41, gaoliming wrote:
> Hi, all
>    If no other comment, I will merge this patch set tomorrow.
> 
> Thanks
> Liming
>> -----邮件原件-----
>> 发件人: Kun Qin <kuqin12@gmail.com>
>> 发送时间: 2022年1月26日 3:39
>> 收件人: devel@edk2.groups.io
>> 抄送: Jian J Wang <jian.j.wang@intel.com>; Liming Gao
>> <gaoliming@byosoft.com.cn>; Hao A Wu <hao.a.wu@intel.com>; Michael D
>> Kinney <michael.d.kinney@intel.com>; Zhiguang Liu
>> <zhiguang.liu@intel.com>; Leif Lindholm <leif@nuviainc.com>; Ard
>> Biesheuvel <ardb+tianocore@kernel.org>; Bret Barkelew
>> <Bret.Barkelew@microsoft.com>; Michael Kubacki
>> <michael.kubacki@microsoft.com>; Sami Mujawar <sami.mujawar@arm.com>
>> 主题: [PATCH v3 0/6] MM communicate functionality in variable policy
>>
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3709
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3751
>>
>> This patch series is a follow up of previous submission:
>> https://edk2.groups.io/g/devel/message/85116
>>
>> All v3 patches added "Reviewed-By" tags collected during v2 series.
>>
>> Patch v3 branch:
>> https://github.com/kuqin12/edk2/tree/mm_communicate_check_v3
>>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Hao A Wu <hao.a.wu@intel.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>> 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>
>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>>
>> Kun Qin (6):
>>    MdeModulePkg: VariableSmmRuntimeDxe: Fix Variable Policy Message
>>      Length
>>    MdePkg: MmCommunication2: Update MM communicate2 function
>> description
>>    ArmPkg: MmCommunicationDxe: MM communicate function argument
>>      attributes
>>    ArmPkg: MmCommunicationDxe: Update MM communicate
>> `CommBuffer**`
>>      checks
>>    ArmPkg: MmCommunicationDxe: Update MM communicate `CommSize`
>> check
>>    ArmPkg: MmCommunicationDxe: Update MM communicate
>> `MessageLength`
>>      check
>>
>>   ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
>> | 46 ++++++++++++--------
>>   MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c |
>> 10 ++---
>>   MdePkg/Include/Protocol/MmCommunication2.h
>> | 13 +++---
>>   3 files changed, 41 insertions(+), 28 deletions(-)
>>
>> --
>> 2.34.1.windows.1
> 
> 
> 
> 
> 
> 
> 
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-01-27 23:05 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-01-25 19:39 [PATCH v3 0/6] MM communicate functionality in variable policy Kun Qin
2022-01-25 19:39 ` [PATCH v3 1/6] MdeModulePkg: VariableSmmRuntimeDxe: Fix Variable Policy Message Length Kun Qin
2022-01-25 19:39 ` [PATCH v3 2/6] MdePkg: MmCommunication2: Update MM communicate2 function description Kun Qin
2022-01-25 19:39 ` [PATCH v3 3/6] ArmPkg: MmCommunicationDxe: MM communicate function argument attributes Kun Qin
2022-01-25 19:39 ` [PATCH v3 4/6] ArmPkg: MmCommunicationDxe: Update MM communicate `CommBuffer**` checks Kun Qin
2022-01-25 19:39 ` [PATCH v3 5/6] ArmPkg: MmCommunicationDxe: Update MM communicate `CommSize` check Kun Qin
2022-01-25 19:39 ` [PATCH v3 6/6] ArmPkg: MmCommunicationDxe: Update MM communicate `MessageLength` check Kun Qin
2022-01-26  1:41 ` 回复: [PATCH v3 0/6] MM communicate functionality in variable policy gaoliming
2022-01-27 23:05   ` [edk2-devel] " Kun Qin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox