Re: [edk2-devel] [PATCH] SecurityPkg: Add RPMC Index to the RpmcLib

["Yao, Jiewen" <jiewen.yao@intel.com>]

Hi Jian
I tend to agree with you.

My point is: we need review the RPMC lib again with a real use case, before we change the API.
So far, I am not sure adding CountIndex is the best idea.
I think the current design is incomplete, with the fact that there is no GetCounterNumber() API.

On the other hand, if you think there is no real use case and we are free to update the API, then why not remove the RpmcLib.h at first?
We can add it back with a solid solution later.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Wang, Jian J <jian.j.wang@intel.com>
> Sent: Monday, November 23, 2020 1:23 PM
> To: gaoliming <gaoliming@byosoft.com.cn>; 'Laszlo Ersek'
> <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> devel@edk2.groups.io; Mistry, Nishant C <nishant.c.mistry@intel.com>
> Cc: afish@apple.com; 'Leif Lindholm' <leif@nuviainc.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] [PATCH] SecurityPkg: Add RPMC Index to the
> RpmcLib
> 
> Liming,
> I'm ok to revert the patch. Thanks doing it.
> 
> Jiewen,
> Currently the RpmcLib has no real users in edk2 master. I don't see reasons
> to keep old interface. Besides, new interface can be used for one RPMC use
> case (e.g. defining a default RPMC index), even if variable protection feature
> just needs one eventually. From potential design change due to security
> consideration, I prefer to just keep new interface to reduce the impact.
> 
> Regards,
> Jian
> 
> > -----Original Message-----
> > From: gaoliming <gaoliming@byosoft.com.cn>
> > Sent: Monday, November 23, 2020 9:01 AM
> > To: 'Laszlo Ersek' <lersek@redhat.com>; Yao, Jiewen
> <jiewen.yao@intel.com>;
> > devel@edk2.groups.io; Wang, Jian J <jian.j.wang@intel.com>; Mistry,
> Nishant C
> > <nishant.c.mistry@intel.com>
> > Cc: afish@apple.com; 'Leif Lindholm' <leif@nuviainc.com>; Kinney, Michael
> D
> > <michael.d.kinney@intel.com>
> > Subject: 回复: [edk2-devel] [PATCH] SecurityPkg: Add RPMC Index to the
> > RpmcLib
> >
> > Thanks for your comments. If no other comments, I will sent the patch to
> revert
> > this patch.
> >
> > Thanks
> > Liming
> > > -----邮件原件-----
> > > 发件人: Laszlo Ersek <lersek@redhat.com>
> > > 发送时间: 2020年11月20日 19:02
> > > 收件人: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
> > > gaoliming@byosoft.com.cn; Wang, Jian J <jian.j.wang@intel.com>; Mistry,
> > > Nishant C <nishant.c.mistry@intel.com>
> > > 抄送: afish@apple.com; 'Leif Lindholm' <leif@nuviainc.com>; Kinney,
> Michael
> > > D <michael.d.kinney@intel.com>
> > > 主题: Re: [edk2-devel] [PATCH] SecurityPkg: Add RPMC Index to the
> RpmcLib
> > >
> > > On 11/20/20 08:11, Yao, Jiewen wrote:
> > > > I agree with Liming.
> > > >
> > > > I recommend we follow the code-freeze process.
> > > > 	"By the date of the soft feature freeze, developers must have sent
> their
> > > patches to the mailing list and received positive maintainer reviews
> > > (Reviewed-by or Acked-by tags)."
> > > >
> > > > The re-design could be compatible in some way. For example, we can
> keep
> > > old API and define RequestMonotonicCounterEx(),
> > > IncrementMonotonicCounterEx().
> > > >
> > > > I am also thinking that we should check in together with a lib consumer
> to
> > > show the design to see what is really needed for the counter index.
> > > >
> > > > So I vote to revert the change.
> > >
> > > I agree. Without knowing many of the details:
> > >
> > > The patch references
> > > <https://bugzilla.tianocore.org/show_bug.cgi?id=2594>, and that ticket
> > > is a TianoCore Feature Request. Additionally, comment#0 on the BZ says:
> > >
> > > "Two more features are needed to be added to non-volatile variable
> > > services [...] This BZ is for RPMC based solution only".
> > >
> > > I think the patch should not have been committed.
> > >
> > > Thanks
> > > Laszlo
> > >
> > > >
> > > >> -----Original Message-----
> > > >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > >> gaoliming
> > > >> Sent: Friday, November 20, 2020 2:55 PM
> > > >> To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io;
> Mistry,
> > > >> Nishant C <nishant.c.mistry@intel.com>
> > > >> Cc: afish@apple.com; lersek@redhat.com; 'Leif Lindholm'
> > > >> <leif@nuviainc.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> > > >> Subject: 回复: [edk2-devel] [PATCH] SecurityPkg: Add RPMC Index to
> the
> > > >> RpmcLib
> > > >>
> > > >> Jian:
> > > >>  The commit message mentions that the re-design requires multiple
> > > RPMC
> > > >> counter usages.
> > > >>  The library API is also updated to support multiple RPMC. So, I think
> this
> > > >> is new feature.
> > > >>
> > > >>  But, this is just my idea. I would like to collect more feedback from the
> > > >> mail list.
> > > >>
> > > >> Thanks
> > > >> Liming
> > > >>> -----邮件原件-----
> > > >>> 发件人: Wang, Jian J <jian.j.wang@intel.com>
> > > >>> 发送时间: 2020年11月20日 14:33
> > > >>> 收件人: devel@edk2.groups.io; gaoliming@byosoft.com.cn; Mistry,
> > > >> Nishant C
> > > >>> <nishant.c.mistry@intel.com>
> > > >>> 主题: RE: [edk2-devel] [PATCH] SecurityPkg: Add RPMC Index to the
> > > >> RpmcLib
> > > >>>
> > > >>> Liming,
> > > >>>
> > > >>> Sorry, I didn't notice it. But the patch was just updating the existing
> > > >> code. It'd
> > > >>> be
> > > >>> more like bug fix than feature, I think.
> > > >>>
> > > >>> Regards,
> > > >>> Jian
> > > >>>
> > > >>>> -----Original Message-----
> > > >>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > >>> gaoliming
> > > >>>> Sent: Friday, November 20, 2020 2:27 PM
> > > >>>> To: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@intel.com>;
> > > Mistry,
> > > >>>> Nishant C <nishant.c.mistry@intel.com>
> > > >>>> Cc: gaoliming@byosoft.com.cn
> > > >>>> Subject: 回复: [edk2-devel] [PATCH] SecurityPkg: Add RPMC Index to
> > > the
> > > >>>> RpmcLib
> > > >>>>
> > > >>>> Jian:
> > > >>>>  This change is like a feature instead of bug fix. Now, we are in soft
> > > >>>> feature freeze phase.
> > > >>>>  According to SFF definition
> > > >>>>
> > > https://github.com/tianocore/tianocore.github.io/wiki/SoftFeatureFreeze,
> > > >>>>  this feature should be deferred to next stable tag.
> > > >>>>
> > > >>>>  So, I suggest to revert this change, and merge it after the stable tag
> > > >>>> 202011.
> > > >>>>
> > > >>>> Thanks
> > > >>>> Liming
> > > >>>>> -----邮件原件-----
> > > >>>>> 发件人: bounce+27952+67669+4905953+8761045@groups.io
> > > >>>>> <bounce+27952+67669+4905953+8761045@groups.io> 代表 Wang,
> > > >>> Jian J
> > > >>>>> 发送时间: 2020年11月18日 11:35
> > > >>>>> 收件人: devel@edk2.groups.io; Mistry, Nishant C
> > > >>>>> <nishant.c.mistry@intel.com>
> > > >>>>> 主题: Re: [edk2-devel] [PATCH] SecurityPkg: Add RPMC Index to the
> > > >>> RpmcLib
> > > >>>>>
> > > >>>>>
> > > >>>>> Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
> > > >>>>>
> > > >>>>> Regards,
> > > >>>>> Jian
> > > >>>>>
> > > >>>>>> -----Original Message-----
> > > >>>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf
> Of
> > > >>> Nishant
> > > >>>>>> Mistry
> > > >>>>>> Sent: Thursday, November 12, 2020 2:49 AM
> > > >>>>>> To: devel@edk2.groups.io
> > > >>>>>> Subject: [edk2-devel] [PATCH] SecurityPkg: Add RPMC Index to the
> > > >>> RpmcLib
> > > >>>>>>
> > > >>>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2594
> > > >>>>>>
> > > >>>>>> The re-design requires multiple RPMC counter usages.
> > > >>>>>> The consumer will be capable of selecting amongst multiple
> counters.
> > > >>>>>>
> > > >>>>>> Signed-off-by: Nishant C Mistry <nishant.c.mistry@intel.com>
> > > >>>>>> ---
> > > >>>>>>  SecurityPkg/Include/Library/RpmcLib.h         | 6 +++++-
> > > >>>>>>  SecurityPkg/Library/RpmcLibNull/RpmcLibNull.c | 6 +++++-
> > > >>>>>>  2 files changed, 10 insertions(+), 2 deletions(-)
> > > >>>>>>
> > > >>>>>> diff --git a/SecurityPkg/Include/Library/RpmcLib.h
> > > >>>>>> b/SecurityPkg/Include/Library/RpmcLib.h
> > > >>>>>> index 5882bfae2f..3c15bce1ce 100644
> > > >>>>>> --- a/SecurityPkg/Include/Library/RpmcLib.h
> > > >>>>>> +++ b/SecurityPkg/Include/Library/RpmcLib.h
> > > >>>>>> @@ -14,6 +14,7 @@ SPDX-License-Identifier: BSD-2-Clause-
> Patent
> > > >>>>>>  /**
> > > >>>>>>    Requests the monotonic counter from the designated RPMC
> > > >>> counter.
> > > >>>>>>
> > > >>>>>> +  @param[in]    CounterIndex            The RPMC index
> > > >>>>>>    @param[out]   CounterValue            A pointer to a
> > > buffer
> > > >>> to
> > > >>>>> store the RPMC
> > > >>>>>> value.
> > > >>>>>>
> > > >>>>>>    @retval       EFI_SUCCESS             The operation
> > > >>> completed
> > > >>>>> successfully.
> > > >>>>>> @@ -23,12 +24,15 @@ SPDX-License-Identifier:
> > > BSD-2-Clause-Patent
> > > >>>>>>  EFI_STATUS
> > > >>>>>>  EFIAPI
> > > >>>>>>  RequestMonotonicCounter (
> > > >>>>>> +  IN  UINT8   CounterIndex,
> > > >>>>>>    OUT UINT32  *CounterValue
> > > >>>>>>    );
> > > >>>>>>
> > > >>>>>>  /**
> > > >>>>>>    Increments the monotonic counter in the SPI flash device by 1.
> > > >>>>>>
> > > >>>>>> +  @param[in]    CounterIndex            The RPMC index
> > > >>>>>> +
> > > >>>>>>    @retval       EFI_SUCCESS             The operation
> > > >>> completed
> > > >>>>> successfully.
> > > >>>>>>    @retval       EFI_DEVICE_ERROR        A device error
> > > >>> occurred
> > > >>>>> while attempting
> > > >>>>>> to update the counter.
> > > >>>>>>    @retval       EFI_UNSUPPORTED         The operation is
> > > >>>>> un-supported.
> > > >>>>>> @@ -36,7 +40,7 @@ RequestMonotonicCounter (
> > > >>>>>>  EFI_STATUS
> > > >>>>>>  EFIAPI
> > > >>>>>>  IncrementMonotonicCounter (
> > > >>>>>> -  VOID
> > > >>>>>> +  IN  UINT8   CounterIndex
> > > >>>>>>    );
> > > >>>>>>
> > > >>>>>>  #endif
> > > >>>>>> diff --git a/SecurityPkg/Library/RpmcLibNull/RpmcLibNull.c
> > > >>>>>> b/SecurityPkg/Library/RpmcLibNull/RpmcLibNull.c
> > > >>>>>> index e1dd09eb10..697e493a7c 100644
> > > >>>>>> --- a/SecurityPkg/Library/RpmcLibNull/RpmcLibNull.c
> > > >>>>>> +++ b/SecurityPkg/Library/RpmcLibNull/RpmcLibNull.c
> > > >>>>>> @@ -12,6 +12,7 @@ SPDX-License-Identifier: BSD-2-Clause-
> Patent
> > > >>>>>>  /**
> > > >>>>>>    Requests the monotonic counter from the designated RPMC
> > > >>> counter.
> > > >>>>>>
> > > >>>>>> +  @param[in]    CounterIndex            The RPMC index
> > > >>>>>>    @param[out]   CounterValue            A pointer to a
> > > buffer
> > > >>> to
> > > >>>>> store the RPMC
> > > >>>>>> value.
> > > >>>>>>
> > > >>>>>>    @retval       EFI_SUCCESS             The operation
> > > >>> completed
> > > >>>>> successfully.
> > > >>>>>> @@ -21,6 +22,7 @@ SPDX-License-Identifier: BSD-2-Clause-
> Patent
> > > >>>>>>  EFI_STATUS
> > > >>>>>>  EFIAPI
> > > >>>>>>  RequestMonotonicCounter (
> > > >>>>>> +  IN  UINT8   CounterIndex,
> > > >>>>>>    OUT UINT32  *CounterValue
> > > >>>>>>    )
> > > >>>>>>  {
> > > >>>>>> @@ -31,6 +33,8 @@ RequestMonotonicCounter (
> > > >>>>>>  /**
> > > >>>>>>    Increments the monotonic counter in the SPI flash device by 1.
> > > >>>>>>
> > > >>>>>> +  @param[in]    CounterIndex            The RPMC index
> > > >>>>>> +
> > > >>>>>>    @retval       EFI_SUCCESS             The operation
> > > >>> completed
> > > >>>>> successfully.
> > > >>>>>>    @retval       EFI_DEVICE_ERROR        A device error
> > > >>> occurred
> > > >>>>> while attempting
> > > >>>>>> to update the counter.
> > > >>>>>>    @retval       EFI_UNSUPPORTED         The operation is
> > > >>>>> un-supported.
> > > >>>>>> @@ -38,7 +42,7 @@ RequestMonotonicCounter (
> > > >>>>>>  EFI_STATUS
> > > >>>>>>  EFIAPI
> > > >>>>>>  IncrementMonotonicCounter (
> > > >>>>>> -  VOID
> > > >>>>>> +  IN  UINT8   CounterIndex
> > > >>>>>>    )
> > > >>>>>>  {
> > > >>>>>>    ASSERT (FALSE);
> > > >>>>>> --
> > > >>>>>> 2.16.2.windows.1
> > > >>>>>>
> > > >>>>>>
> > > >>>>>>
> > > >>>>>>
> > > >>>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>
> > > >>
> > > >>
> > > >>
> > > >>
> > > >> 
> > > >>
> > > >
> >
> >