[edk2-devel] [PATCH] SecurityPkg: Add RPMC Index to the RpmcLib
Yao, Jiewen
jiewen.yao at intel.com
Mon Nov 23 06:58:49 UTC 2020
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 at intel.com>
> Sent: Monday, November 23, 2020 1:23 PM
> To: gaoliming <gaoliming at byosoft.com.cn>; 'Laszlo Ersek'
> <lersek at redhat.com>; Yao, Jiewen <jiewen.yao at intel.com>;
> devel at edk2.groups.io; Mistry, Nishant C <nishant.c.mistry at intel.com>
> Cc: afish at apple.com; 'Leif Lindholm' <leif at nuviainc.com>; Kinney, Michael D
> <michael.d.kinney at 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 at byosoft.com.cn>
> > Sent: Monday, November 23, 2020 9:01 AM
> > To: 'Laszlo Ersek' <lersek at redhat.com>; Yao, Jiewen
> <jiewen.yao at intel.com>;
> > devel at edk2.groups.io; Wang, Jian J <jian.j.wang at intel.com>; Mistry,
> Nishant C
> > <nishant.c.mistry at intel.com>
> > Cc: afish at apple.com; 'Leif Lindholm' <leif at nuviainc.com>; Kinney, Michael
> D
> > <michael.d.kinney at 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 at redhat.com>
> > > 发送时间: 2020年11月20日 19:02
> > > 收件人: Yao, Jiewen <jiewen.yao at intel.com>; devel at edk2.groups.io;
> > > gaoliming at byosoft.com.cn; Wang, Jian J <jian.j.wang at intel.com>; Mistry,
> > > Nishant C <nishant.c.mistry at intel.com>
> > > 抄送: afish at apple.com; 'Leif Lindholm' <leif at nuviainc.com>; Kinney,
> Michael
> > > D <michael.d.kinney at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
> > > >> gaoliming
> > > >> Sent: Friday, November 20, 2020 2:55 PM
> > > >> To: Wang, Jian J <jian.j.wang at intel.com>; devel at edk2.groups.io;
> Mistry,
> > > >> Nishant C <nishant.c.mistry at intel.com>
> > > >> Cc: afish at apple.com; lersek at redhat.com; 'Leif Lindholm'
> > > >> <leif at nuviainc.com>; Kinney, Michael D <michael.d.kinney at 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 at intel.com>
> > > >>> 发送时间: 2020年11月20日 14:33
> > > >>> 收件人: devel at edk2.groups.io; gaoliming at byosoft.com.cn; Mistry,
> > > >> Nishant C
> > > >>> <nishant.c.mistry at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
> > > >>> gaoliming
> > > >>>> Sent: Friday, November 20, 2020 2:27 PM
> > > >>>> To: devel at edk2.groups.io; Wang, Jian J <jian.j.wang at intel.com>;
> > > Mistry,
> > > >>>> Nishant C <nishant.c.mistry at intel.com>
> > > >>>> Cc: gaoliming at 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 at groups.io
> > > >>>>> <bounce+27952+67669+4905953+8761045 at groups.io> 代表 Wang,
> > > >>> Jian J
> > > >>>>> 发送时间: 2020年11月18日 11:35
> > > >>>>> 收件人: devel at edk2.groups.io; Mistry, Nishant C
> > > >>>>> <nishant.c.mistry at intel.com>
> > > >>>>> 主题: Re: [edk2-devel] [PATCH] SecurityPkg: Add RPMC Index to the
> > > >>> RpmcLib
> > > >>>>>
> > > >>>>>
> > > >>>>> Reviewed-by: Jian J Wang <jian.j.wang at intel.com>
> > > >>>>>
> > > >>>>> Regards,
> > > >>>>> Jian
> > > >>>>>
> > > >>>>>> -----Original Message-----
> > > >>>>>> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf
> Of
> > > >>> Nishant
> > > >>>>>> Mistry
> > > >>>>>> Sent: Thursday, November 12, 2020 2:49 AM
> > > >>>>>> To: devel at 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 at 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
> > > >>>>>>
> > > >>>>>>
> > > >>>>>>
> > > >>>>>>
> > > >>>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>
> > > >>
> > > >>
> > > >>
> > > >>
> > > >>
> > > >>
> > > >
> >
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67791): https://edk2.groups.io/g/devel/message/67791
Mute This Topic: https://groups.io/mt/78447505/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list