[PATCH v2 0/6] MM communicate functionality in variable policy

[PATCH v2 0/6] MM communicate functionality in variable policy

[Kun Qin]

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/84140

v2 patches mainly focus on feedback for commits submitted in v1 patches:
a. Splitted the original ArmPkg patch into 4 separate patches;
b. Updated patches according to Uncrustify scanning results;

Patch v2 branch: https://github.com/kuqin12/edk2/tree/mm_communicate_check_v2

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>

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.32.0.windows.1

[PATCH v2 1/6] MdeModulePkg: VariableSmmRuntimeDxe: Fix Variable Policy Message Length

[Kun Qin]

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>
---

Notes:
    v2:
    - No review, no updates

 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.32.0.windows.1

回复: [edk2-devel] [PATCH v2 1/6] MdeModulePkg: VariableSmmRuntimeDxe: Fix Variable Policy Message Length

[gaoliming]

Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>

> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Kun Qin
> 发送时间: 2021年12月21日 9:33
> 收件人: 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>; Bret
> Barkelew <Bret.Barkelew@microsoft.com>; Michael Kubacki
> <michael.kubacki@microsoft.com>
> 主题: [edk2-devel] [PATCH v2 1/6] MdeModulePkg: VariableSmmRuntimeDxe:
> Fix Variable Policy Message Length
> 
> 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>
> ---
> 
> Notes:
>     v2:
>     - No review, no updates
> 
>  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.32.0.windows.1
> 
> 
> 
> 
> 

[PATCH v2 2/6] MdePkg: MmCommunication2: Update MM communicate2 function description

[Kun Qin]

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>
---

Notes:
    v2:
    - Newly added

 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.32.0.windows.1

回复: [edk2-devel] [PATCH v2 2/6] MdePkg: MmCommunication2: Update MM communicate2 function description

[gaoliming]

Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>

> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Kun Qin
> 发送时间: 2021年12月21日 9:34
> 收件人: devel@edk2.groups.io
> 抄送: Michael D Kinney <michael.d.kinney@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Zhiguang Liu <zhiguang.liu@intel.com>
> 主题: [edk2-devel] [PATCH v2 2/6] MdePkg: MmCommunication2: Update
> MM communicate2 function description
> 
> 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>
> ---
> 
> Notes:
>     v2:
>     - Newly added
> 
>  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.32.0.windows.1
> 
> 
> 
> 
> 

[PATCH v2 3/6] ArmPkg: MmCommunicationDxe: MM communicate function argument attributes

[Kun Qin]

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>

Signed-off-by: Kun Qin <kuqin12@gmail.com>
---

Notes:
    v2:
    - Splitting patch into 1 of 4 [Ard]

 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.32.0.windows.1

[PATCH v2 4/6] ArmPkg: MmCommunicationDxe: Update MM communicate `CommBuffer**` checks

[Kun Qin]

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>

Signed-off-by: Kun Qin <kuqin12@gmail.com>
---

Notes:
    v2:
    - Splitting patch into 2 of 4 [Ard]
    - Uncrustify style update

 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.32.0.windows.1

[PATCH v2 5/6] ArmPkg: MmCommunicationDxe: Update MM communicate `CommSize` check

[Kun Qin]

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>

Signed-off-by: Kun Qin <kuqin12@gmail.com>
---

Notes:
    v2:
    - Splitting patch into 3 of 4 [Ard]

 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.32.0.windows.1

[PATCH v2 6/6] ArmPkg: MmCommunicationDxe: Update MM communicate `MessageLength` check

[Kun Qin]

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>

Signed-off-by: Kun Qin <kuqin12@gmail.com>
---

Notes:
    v2:
    - Splitting patch into 4 of 4 [Ard]
    - Uncrustify style update

 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.32.0.windows.1

Re: [edk2-devel] [PATCH v2 0/6] MM communicate functionality in variable policy

[Kun Qin]

[-- Attachment #1: Type: text/plain, Size: 198 bytes --]

Hi ArmPkg maintainers,

It has been almost a month since this v2 patch series has been sent out for review. Could you please take a look and provide feedback? Thanks in advance.

Regards,
Kun

[-- Attachment #2: Type: text/html, Size: 218 bytes --]

Re: [edk2-devel] [PATCH v2 0/6] MM communicate functionality in variable policy

[Rebecca Cran]

[-- Attachment #1: Type: text/plain, Size: 397 bytes --]

[+Leif, Ard]


Kun,


There's often so much mailing list traffic that you need to CC the 
maintainers for them to see your emails.


-- 

Rebecca Cran


On 1/18/22 11:40, Kun Qin wrote:
> Hi ArmPkg maintainers,
>
> It has been almost a month since this v2 patch series has been sent 
> out for review. Could you please take a look and provide feedback? 
> Thanks in advance.
>
> Regards,
> Kun
> 

[-- Attachment #2: Type: text/html, Size: 953 bytes --]

Re: [edk2-devel] [PATCH v2 0/6] MM communicate functionality in variable policy

[Kun Qin]

I thought the reply on website will retain the original CC list. But I 
was wrong about that.

Thanks for the reminder, Rebecca.

Regards,
Kun

On 01/18/2022 14:04, Rebecca Cran wrote:
> [+Leif, Ard]
> 
> 
> Kun,
> 
> 
> There's often so much mailing list traffic that you need to CC the 
> maintainers for them to see your emails.
> 
> 
> -- 
> 
> Rebecca Cran
> 
> 
> On 1/18/22 11:40, Kun Qin wrote:
>> Hi ArmPkg maintainers,
>>
>> It has been almost a month since this v2 patch series has been sent 
>> out for review. Could you please take a look and provide feedback? 
>> Thanks in advance.
>>
>> Regards,
>> Kun
>> 

Re: [edk2-devel] [PATCH v2 0/6] MM communicate functionality in variable policy

[Sami Mujawar]

Hi Kun,

I will review this patch series shortly.

Regards,

Sami Mujawar

On 18/01/2022, 22:07, "devel@edk2.groups.io on behalf of Kun Qin via groups.io" <devel@edk2.groups.io on behalf of kuqin12=gmail.com@groups.io> wrote:

    I thought the reply on website will retain the original CC list. But I 
    was wrong about that.

    Thanks for the reminder, Rebecca.

    Regards,
    Kun

    On 01/18/2022 14:04, Rebecca Cran wrote:
    > [+Leif, Ard]
    > 
    > 
    > Kun,
    > 
    > 
    > There's often so much mailing list traffic that you need to CC the 
    > maintainers for them to see your emails.
    > 
    > 
    > -- 
    > 
    > Rebecca Cran
    > 
    > 
    > On 1/18/22 11:40, Kun Qin wrote:
    >> Hi ArmPkg maintainers,
    >>
    >> It has been almost a month since this v2 patch series has been sent 
    >> out for review. Could you please take a look and provide feedback? 
    >> Thanks in advance.
    >>
    >> Regards,
    >> Kun
    >> 


    

Re: [edk2-devel] [PATCH v2 0/6] MM communicate functionality in variable policy

[Sami Mujawar]

Hi Kun,

Thank you for this patch series.

These changes look good to me. For this series

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

On 21/12/2021 01:33 AM, Kun Qin via groups.io wrote:
> 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/84140
>
> v2 patches mainly focus on feedback for commits submitted in v1 patches:
> a. Splitted the original ArmPkg patch into 4 separate patches;
> b. Updated patches according to Uncrustify scanning results;
>
> Patch v2 branch: https://github.com/kuqin12/edk2/tree/mm_communicate_check_v2
>
> 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>
>
> 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(-)
>