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

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

[Kun Qin]

Hi ArmPkg and MdeModulePkg maintainers,

It has been a week since the patches were sent. Could you please review 
the changes and let me know if there is any feedback? Any input is 
appreciated.

Regards,
Kun

On 11/29/2021 16:39, 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
> 
> Currently, setups with variable policy operations used together with MM
> communicate from ArmPkg could fail with `EFI_INVALID_PARAMETER`. This was
> due to the errors from 2 following aspects:
> 
> 1. For variable policy implementations in MdeModulePkg, the DXE runtime
> agent would communicate to MM to disable, register or query policies.
> However, during these operations, the MessageLength calculation is
> including MM communicate header. This could lead to MM agent read data
> across the given buffer boundary and/or trigger other errors.
> 
> 2. On the other hand, current MM communicate routine from ArmPkg would
> fail the function if the input message length does not equal to input
> buffer size.
> 
> As defined in PI specification, the `CommSize`, when as input, should
> stand for "The size of the data buffer being passed in", which would mean
> the maximal number of bytes `CommBuffer` can hold. In turn, the value of
> this input parameter can be used for MM handlers to determine whether the
> output data is too large to fit in this buffer. Enforcing the incoming
> buffer to hold exactly the number of used bytes mismatches with the PI
> spec description.
> 
> This change fix MessageLength field calculation from variable policy and
> updated input argument inspections from MM communicate routine in ArmPkg
> to match PI spec descriptions.
> 
> Patch v1 branch: https://github.com/kuqin12/edk2/tree/mm_communicate_check
> 
> 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: 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 (2):
>    MdeModulePkg: VariableSmmRuntimeDxe: Fix Variable Policy Message
>      Length
>    ArmPkg: MmCommunicationDxe: Update MM communicate input arguments
>      checks
> 
>   ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c               | 44 ++++++++++++--------
>   MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c | 10 ++---
>   2 files changed, 32 insertions(+), 22 deletions(-)
> 

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

[Ard Biesheuvel]

On Mon, 6 Dec 2021 at 19:35, Kun Qin <kuqin12@gmail.com> wrote:
>
> Hi ArmPkg and MdeModulePkg maintainers,
>
> It has been a week since the patches were sent. Could you please review
> the changes and let me know if there is any feedback? Any input is
> appreciated.
>

As far as I know, we are still in hard freeze for the upcoming stable tag.

>
> On 11/29/2021 16:39, 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
> >
> > Currently, setups with variable policy operations used together with MM
> > communicate from ArmPkg could fail with `EFI_INVALID_PARAMETER`. This was
> > due to the errors from 2 following aspects:
> >
> > 1. For variable policy implementations in MdeModulePkg, the DXE runtime
> > agent would communicate to MM to disable, register or query policies.
> > However, during these operations, the MessageLength calculation is
> > including MM communicate header. This could lead to MM agent read data
> > across the given buffer boundary and/or trigger other errors.
> >
> > 2. On the other hand, current MM communicate routine from ArmPkg would
> > fail the function if the input message length does not equal to input
> > buffer size.
> >
> > As defined in PI specification, the `CommSize`, when as input, should
> > stand for "The size of the data buffer being passed in", which would mean
> > the maximal number of bytes `CommBuffer` can hold. In turn, the value of
> > this input parameter can be used for MM handlers to determine whether the
> > output data is too large to fit in this buffer. Enforcing the incoming
> > buffer to hold exactly the number of used bytes mismatches with the PI
> > spec description.
> >
> > This change fix MessageLength field calculation from variable policy and
> > updated input argument inspections from MM communicate routine in ArmPkg
> > to match PI spec descriptions.
> >
> > Patch v1 branch: https://github.com/kuqin12/edk2/tree/mm_communicate_check
> >
> > 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: 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 (2):
> >    MdeModulePkg: VariableSmmRuntimeDxe: Fix Variable Policy Message
> >      Length
> >    ArmPkg: MmCommunicationDxe: Update MM communicate input arguments
> >      checks
> >
> >   ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c               | 44 ++++++++++++--------
> >   MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c | 10 ++---
> >   2 files changed, 32 insertions(+), 22 deletions(-)
> >

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

[Kun Qin]

Thanks for the information, Ard. I just meant to plan ahead so that I 
can work on the feedback for these patches, if any.

I can ping back the thread again once the stable tag is created.

Regards,
Kun

On 12/06/2021 10:41, Ard Biesheuvel wrote:
> On Mon, 6 Dec 2021 at 19:35, Kun Qin <kuqin12@gmail.com> wrote:
>>
>> Hi ArmPkg and MdeModulePkg maintainers,
>>
>> It has been a week since the patches were sent. Could you please review
>> the changes and let me know if there is any feedback? Any input is
>> appreciated.
>>
> 
> As far as I know, we are still in hard freeze for the upcoming stable tag.
> 
>>
>> On 11/29/2021 16:39, 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
>>>
>>> Currently, setups with variable policy operations used together with MM
>>> communicate from ArmPkg could fail with `EFI_INVALID_PARAMETER`. This was
>>> due to the errors from 2 following aspects:
>>>
>>> 1. For variable policy implementations in MdeModulePkg, the DXE runtime
>>> agent would communicate to MM to disable, register or query policies.
>>> However, during these operations, the MessageLength calculation is
>>> including MM communicate header. This could lead to MM agent read data
>>> across the given buffer boundary and/or trigger other errors.
>>>
>>> 2. On the other hand, current MM communicate routine from ArmPkg would
>>> fail the function if the input message length does not equal to input
>>> buffer size.
>>>
>>> As defined in PI specification, the `CommSize`, when as input, should
>>> stand for "The size of the data buffer being passed in", which would mean
>>> the maximal number of bytes `CommBuffer` can hold. In turn, the value of
>>> this input parameter can be used for MM handlers to determine whether the
>>> output data is too large to fit in this buffer. Enforcing the incoming
>>> buffer to hold exactly the number of used bytes mismatches with the PI
>>> spec description.
>>>
>>> This change fix MessageLength field calculation from variable policy and
>>> updated input argument inspections from MM communicate routine in ArmPkg
>>> to match PI spec descriptions.
>>>
>>> Patch v1 branch: https://github.com/kuqin12/edk2/tree/mm_communicate_check
>>>
>>> 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: 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 (2):
>>>     MdeModulePkg: VariableSmmRuntimeDxe: Fix Variable Policy Message
>>>       Length
>>>     ArmPkg: MmCommunicationDxe: Update MM communicate input arguments
>>>       checks
>>>
>>>    ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c               | 44 ++++++++++++--------
>>>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c | 10 ++---
>>>    2 files changed, 32 insertions(+), 22 deletions(-)
>>>

回复: [edk2-devel] [PATCH v1 0/2] MM communicate functionality in variable policy

[gaoliming]

Kun:
  Does this change impact current behavior? Seemly, there is no strict check on MessageLength.

Thanks
Liming
> -----邮件原件-----
> 发件人: Kun Qin <kuqin12@gmail.com>
> 发送时间: 2021年12月7日 2:47
> 收件人: Ard Biesheuvel <ardb@kernel.org>
> 抄送: edk2-devel-groups-io <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>; Leif Lindholm <leif@nuviainc.com>; Ard
> Biesheuvel <ardb+tianocore@kernel.org>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Michael Kubacki
> <michael.kubacki@microsoft.com>
> 主题: Re: [edk2-devel] [PATCH v1 0/2] MM communicate functionality in
> variable policy
> 
> Thanks for the information, Ard. I just meant to plan ahead so that I
> can work on the feedback for these patches, if any.
> 
> I can ping back the thread again once the stable tag is created.
> 
> Regards,
> Kun
> 
> On 12/06/2021 10:41, Ard Biesheuvel wrote:
> > On Mon, 6 Dec 2021 at 19:35, Kun Qin <kuqin12@gmail.com> wrote:
> >>
> >> Hi ArmPkg and MdeModulePkg maintainers,
> >>
> >> It has been a week since the patches were sent. Could you please review
> >> the changes and let me know if there is any feedback? Any input is
> >> appreciated.
> >>
> >
> > As far as I know, we are still in hard freeze for the upcoming stable tag.
> >
> >>
> >> On 11/29/2021 16:39, 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
> >>>
> >>> Currently, setups with variable policy operations used together with MM
> >>> communicate from ArmPkg could fail with `EFI_INVALID_PARAMETER`.
> This was
> >>> due to the errors from 2 following aspects:
> >>>
> >>> 1. For variable policy implementations in MdeModulePkg, the DXE
> runtime
> >>> agent would communicate to MM to disable, register or query policies.
> >>> However, during these operations, the MessageLength calculation is
> >>> including MM communicate header. This could lead to MM agent read
> data
> >>> across the given buffer boundary and/or trigger other errors.
> >>>
> >>> 2. On the other hand, current MM communicate routine from ArmPkg
> would
> >>> fail the function if the input message length does not equal to input
> >>> buffer size.
> >>>
> >>> As defined in PI specification, the `CommSize`, when as input, should
> >>> stand for "The size of the data buffer being passed in", which would mean
> >>> the maximal number of bytes `CommBuffer` can hold. In turn, the value of
> >>> this input parameter can be used for MM handlers to determine whether
> the
> >>> output data is too large to fit in this buffer. Enforcing the incoming
> >>> buffer to hold exactly the number of used bytes mismatches with the PI
> >>> spec description.
> >>>
> >>> This change fix MessageLength field calculation from variable policy and
> >>> updated input argument inspections from MM communicate routine in
> ArmPkg
> >>> to match PI spec descriptions.
> >>>
> >>> Patch v1 branch:
> https://github.com/kuqin12/edk2/tree/mm_communicate_check
> >>>
> >>> 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: 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 (2):
> >>>     MdeModulePkg: VariableSmmRuntimeDxe: Fix Variable Policy
> Message
> >>>       Length
> >>>     ArmPkg: MmCommunicationDxe: Update MM communicate input
> arguments
> >>>       checks
> >>>
> >>>    ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> | 44 ++++++++++++--------
> >>>
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c |
> 10 ++---
> >>>    2 files changed, 32 insertions(+), 22 deletions(-)
> >>>

Re: 回复: [edk2-devel] [PATCH v1 0/2] MM communicate functionality in variable policy

[Kun Qin]

Hi Liming,

The strict check on MessageLength is only existent in `MmCommunication` 
driver from ArmPkg 
(https://github.com/tianocore/edk2/blob/e1e7306b54147e65cb7347b060e94f336d4a82d2/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c#L110). 
The x86 version does not seem to check MessageLength.

So the behavior does not change on X86 systems, but the MdeModulePkg 
change alone will fix the failure to proceed with variable policy on Arm 
system.

We also proposed a change in ArmPkg regarding the MessageLength check to 
stick closer to PI spec in this patch series.

Regards,
Kun

On 12/06/2021 17:12, gaoliming wrote:
> Kun:
>    Does this change impact current behavior? Seemly, there is no strict check on MessageLength.
> 
> Thanks
> Liming
>> -----邮件原件-----
>> 发件人: Kun Qin <kuqin12@gmail.com>
>> 发送时间: 2021年12月7日 2:47
>> 收件人: Ard Biesheuvel <ardb@kernel.org>
>> 抄送: edk2-devel-groups-io <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>; Leif Lindholm <leif@nuviainc.com>; Ard
>> Biesheuvel <ardb+tianocore@kernel.org>; Bret Barkelew
>> <Bret.Barkelew@microsoft.com>; Michael Kubacki
>> <michael.kubacki@microsoft.com>
>> 主题: Re: [edk2-devel] [PATCH v1 0/2] MM communicate functionality in
>> variable policy
>>
>> Thanks for the information, Ard. I just meant to plan ahead so that I
>> can work on the feedback for these patches, if any.
>>
>> I can ping back the thread again once the stable tag is created.
>>
>> Regards,
>> Kun
>>
>> On 12/06/2021 10:41, Ard Biesheuvel wrote:
>>> On Mon, 6 Dec 2021 at 19:35, Kun Qin <kuqin12@gmail.com> wrote:
>>>>
>>>> Hi ArmPkg and MdeModulePkg maintainers,
>>>>
>>>> It has been a week since the patches were sent. Could you please review
>>>> the changes and let me know if there is any feedback? Any input is
>>>> appreciated.
>>>>
>>>
>>> As far as I know, we are still in hard freeze for the upcoming stable tag.
>>>
>>>>
>>>> On 11/29/2021 16:39, 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
>>>>>
>>>>> Currently, setups with variable policy operations used together with MM
>>>>> communicate from ArmPkg could fail with `EFI_INVALID_PARAMETER`.
>> This was
>>>>> due to the errors from 2 following aspects:
>>>>>
>>>>> 1. For variable policy implementations in MdeModulePkg, the DXE
>> runtime
>>>>> agent would communicate to MM to disable, register or query policies.
>>>>> However, during these operations, the MessageLength calculation is
>>>>> including MM communicate header. This could lead to MM agent read
>> data
>>>>> across the given buffer boundary and/or trigger other errors.
>>>>>
>>>>> 2. On the other hand, current MM communicate routine from ArmPkg
>> would
>>>>> fail the function if the input message length does not equal to input
>>>>> buffer size.
>>>>>
>>>>> As defined in PI specification, the `CommSize`, when as input, should
>>>>> stand for "The size of the data buffer being passed in", which would mean
>>>>> the maximal number of bytes `CommBuffer` can hold. In turn, the value of
>>>>> this input parameter can be used for MM handlers to determine whether
>> the
>>>>> output data is too large to fit in this buffer. Enforcing the incoming
>>>>> buffer to hold exactly the number of used bytes mismatches with the PI
>>>>> spec description.
>>>>>
>>>>> This change fix MessageLength field calculation from variable policy and
>>>>> updated input argument inspections from MM communicate routine in
>> ArmPkg
>>>>> to match PI spec descriptions.
>>>>>
>>>>> Patch v1 branch:
>> https://github.com/kuqin12/edk2/tree/mm_communicate_check
>>>>>
>>>>> 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: 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 (2):
>>>>>      MdeModulePkg: VariableSmmRuntimeDxe: Fix Variable Policy
>> Message
>>>>>        Length
>>>>>      ArmPkg: MmCommunicationDxe: Update MM communicate input
>> arguments
>>>>>        checks
>>>>>
>>>>>     ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
>> | 44 ++++++++++++--------
>>>>>
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c |
>> 10 ++---
>>>>>     2 files changed, 32 insertions(+), 22 deletions(-)
>>>>>
> 
> 
> 
> 
> 
> 
> 

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

[Kun Qin]

Hi ArmPkg and MdeModulePkg maintainers,

Now that the hard freeze is lifted, could you please provide some 
feedback on these patches when you have a chance?

Thanks in advance.

Regards,
Kun

On 12/06/2021 10:41, Ard Biesheuvel wrote:
> On Mon, 6 Dec 2021 at 19:35, Kun Qin <kuqin12@gmail.com> wrote:
>>
>> Hi ArmPkg and MdeModulePkg maintainers,
>>
>> It has been a week since the patches were sent. Could you please review
>> the changes and let me know if there is any feedback? Any input is
>> appreciated.
>>
> 
> As far as I know, we are still in hard freeze for the upcoming stable tag.
> 
>>
>> On 11/29/2021 16:39, 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
>>>
>>> Currently, setups with variable policy operations used together with MM
>>> communicate from ArmPkg could fail with `EFI_INVALID_PARAMETER`. This was
>>> due to the errors from 2 following aspects:
>>>
>>> 1. For variable policy implementations in MdeModulePkg, the DXE runtime
>>> agent would communicate to MM to disable, register or query policies.
>>> However, during these operations, the MessageLength calculation is
>>> including MM communicate header. This could lead to MM agent read data
>>> across the given buffer boundary and/or trigger other errors.
>>>
>>> 2. On the other hand, current MM communicate routine from ArmPkg would
>>> fail the function if the input message length does not equal to input
>>> buffer size.
>>>
>>> As defined in PI specification, the `CommSize`, when as input, should
>>> stand for "The size of the data buffer being passed in", which would mean
>>> the maximal number of bytes `CommBuffer` can hold. In turn, the value of
>>> this input parameter can be used for MM handlers to determine whether the
>>> output data is too large to fit in this buffer. Enforcing the incoming
>>> buffer to hold exactly the number of used bytes mismatches with the PI
>>> spec description.
>>>
>>> This change fix MessageLength field calculation from variable policy and
>>> updated input argument inspections from MM communicate routine in ArmPkg
>>> to match PI spec descriptions.
>>>
>>> Patch v1 branch: https://github.com/kuqin12/edk2/tree/mm_communicate_check
>>>
>>> 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: 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 (2):
>>>     MdeModulePkg: VariableSmmRuntimeDxe: Fix Variable Policy Message
>>>       Length
>>>     ArmPkg: MmCommunicationDxe: Update MM communicate input arguments
>>>       checks
>>>
>>>    ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c               | 44 ++++++++++++--------
>>>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c | 10 ++---
>>>    2 files changed, 32 insertions(+), 22 deletions(-)
>>>

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

[Ard Biesheuvel]

On Mon, 13 Dec 2021 at 19:23, Kun Qin <kuqin12@gmail.com> wrote:
>
> Hi ArmPkg and MdeModulePkg maintainers,
>
> Now that the hard freeze is lifted, could you please provide some
> feedback on these patches when you have a chance?
>

My apologies for not responding to these changes.

If these patches are still relevant, please rebase and resend them.


> On 12/06/2021 10:41, Ard Biesheuvel wrote:
> > On Mon, 6 Dec 2021 at 19:35, Kun Qin <kuqin12@gmail.com> wrote:
> >>
> >> Hi ArmPkg and MdeModulePkg maintainers,
> >>
> >> It has been a week since the patches were sent. Could you please review
> >> the changes and let me know if there is any feedback? Any input is
> >> appreciated.
> >>
> >
> > As far as I know, we are still in hard freeze for the upcoming stable tag.
> >
> >>
> >> On 11/29/2021 16:39, 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
> >>>
> >>> Currently, setups with variable policy operations used together with MM
> >>> communicate from ArmPkg could fail with `EFI_INVALID_PARAMETER`. This was
> >>> due to the errors from 2 following aspects:
> >>>
> >>> 1. For variable policy implementations in MdeModulePkg, the DXE runtime
> >>> agent would communicate to MM to disable, register or query policies.
> >>> However, during these operations, the MessageLength calculation is
> >>> including MM communicate header. This could lead to MM agent read data
> >>> across the given buffer boundary and/or trigger other errors.
> >>>
> >>> 2. On the other hand, current MM communicate routine from ArmPkg would
> >>> fail the function if the input message length does not equal to input
> >>> buffer size.
> >>>
> >>> As defined in PI specification, the `CommSize`, when as input, should
> >>> stand for "The size of the data buffer being passed in", which would mean
> >>> the maximal number of bytes `CommBuffer` can hold. In turn, the value of
> >>> this input parameter can be used for MM handlers to determine whether the
> >>> output data is too large to fit in this buffer. Enforcing the incoming
> >>> buffer to hold exactly the number of used bytes mismatches with the PI
> >>> spec description.
> >>>
> >>> This change fix MessageLength field calculation from variable policy and
> >>> updated input argument inspections from MM communicate routine in ArmPkg
> >>> to match PI spec descriptions.
> >>>
> >>> Patch v1 branch: https://github.com/kuqin12/edk2/tree/mm_communicate_check
> >>>
> >>> 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: 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 (2):
> >>>     MdeModulePkg: VariableSmmRuntimeDxe: Fix Variable Policy Message
> >>>       Length
> >>>     ArmPkg: MmCommunicationDxe: Update MM communicate input arguments
> >>>       checks
> >>>
> >>>    ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c               | 44 ++++++++++++--------
> >>>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c | 10 ++---
> >>>    2 files changed, 32 insertions(+), 22 deletions(-)
> >>>

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

[Kun Qin]

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

Hi Ard,

No worries. Liming has helped to merge these patches here: MM 
communicate functionality in variable policy by lgao4 · Pull Request 
#2443 · tianocore/edk2 (github.com) 
<https://github.com/tianocore/edk2/pull/2443>. Thanks for checking back.

Regards,
Kun

On 9/7/2022 1:27 AM, Ard Biesheuvel wrote:
> On Mon, 13 Dec 2021 at 19:23, Kun Qin<kuqin12@gmail.com>  wrote:
>> Hi ArmPkg and MdeModulePkg maintainers,
>>
>> Now that the hard freeze is lifted, could you please provide some
>> feedback on these patches when you have a chance?
>>
> My apologies for not responding to these changes.
>
> If these patches are still relevant, please rebase and resend them.
>
>
>> On 12/06/2021 10:41, Ard Biesheuvel wrote:
>>> On Mon, 6 Dec 2021 at 19:35, Kun Qin<kuqin12@gmail.com>  wrote:
>>>> Hi ArmPkg and MdeModulePkg maintainers,
>>>>
>>>> It has been a week since the patches were sent. Could you please review
>>>> the changes and let me know if there is any feedback? Any input is
>>>> appreciated.
>>>>
>>> As far as I know, we are still in hard freeze for the upcoming stable tag.
>>>
>>>> On 11/29/2021 16:39, 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
>>>>>
>>>>> Currently, setups with variable policy operations used together with MM
>>>>> communicate from ArmPkg could fail with `EFI_INVALID_PARAMETER`. This was
>>>>> due to the errors from 2 following aspects:
>>>>>
>>>>> 1. For variable policy implementations in MdeModulePkg, the DXE runtime
>>>>> agent would communicate to MM to disable, register or query policies.
>>>>> However, during these operations, the MessageLength calculation is
>>>>> including MM communicate header. This could lead to MM agent read data
>>>>> across the given buffer boundary and/or trigger other errors.
>>>>>
>>>>> 2. On the other hand, current MM communicate routine from ArmPkg would
>>>>> fail the function if the input message length does not equal to input
>>>>> buffer size.
>>>>>
>>>>> As defined in PI specification, the `CommSize`, when as input, should
>>>>> stand for "The size of the data buffer being passed in", which would mean
>>>>> the maximal number of bytes `CommBuffer` can hold. In turn, the value of
>>>>> this input parameter can be used for MM handlers to determine whether the
>>>>> output data is too large to fit in this buffer. Enforcing the incoming
>>>>> buffer to hold exactly the number of used bytes mismatches with the PI
>>>>> spec description.
>>>>>
>>>>> This change fix MessageLength field calculation from variable policy and
>>>>> updated input argument inspections from MM communicate routine in ArmPkg
>>>>> to match PI spec descriptions.
>>>>>
>>>>> Patch v1 branch:https://github.com/kuqin12/edk2/tree/mm_communicate_check
>>>>>
>>>>> 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: 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 (2):
>>>>>      MdeModulePkg: VariableSmmRuntimeDxe: Fix Variable Policy Message
>>>>>        Length
>>>>>      ArmPkg: MmCommunicationDxe: Update MM communicate input arguments
>>>>>        checks
>>>>>
>>>>>     ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c               | 44 ++++++++++++--------
>>>>>     MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c | 10 ++---
>>>>>     2 files changed, 32 insertions(+), 22 deletions(-)
>>>>>

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