Re: [PATCH v3 1/3] SecurityPkg: add RpmcLib and VariableKeyLib public headers

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

Thanks Jian. Some comments and thought.

1) RPMC spec uses CounterAddress as the indicator. Can we use the same name instead of CounterId in the API definition?

2) How the caller known which CounterAddress it need to fill?
Do we need an API such as GetValidCounterAddress() ?

3) What is the value to add CounterAddress? Do can we guarantee that 2 drivers use 2 different counter address?
Or If 2 drivers may use same counter address, why not remove the counter address parameter at all ?

Thank you
Yao Jiewen



> -----Original Message-----
> From: Wang, Jian J <jian.j.wang@intel.com>
> Sent: Wednesday, March 18, 2020 10:13 AM
> To: devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B
> <chao.b.zhang@intel.com>; Mistry, Nishant C <nishant.c.mistry@intel.com>
> Subject: [PATCH v3 1/3] SecurityPkg: add RpmcLib and VariableKeyLib public
> headers
> 
> > v3: update retval description in RpmcLib.h
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2594
> 
> RpmcLib.h and VariableKeyLib.h are header files required to access RPMC
> device and Key generator from platform. They will be used to ensure the
> integrity and confidentiality of NV variables.
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Cc: Nishant C Mistry <nishant.c.mistry@intel.com>
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  SecurityPkg/Include/Library/RpmcLib.h        | 46 +++++++++++++++
>  SecurityPkg/Include/Library/VariableKeyLib.h | 59 ++++++++++++++++++++
>  SecurityPkg/SecurityPkg.dec                  |  8 +++
>  3 files changed, 113 insertions(+)
>  create mode 100644 SecurityPkg/Include/Library/RpmcLib.h
>  create mode 100644 SecurityPkg/Include/Library/VariableKeyLib.h
> 
> diff --git a/SecurityPkg/Include/Library/RpmcLib.h
> b/SecurityPkg/Include/Library/RpmcLib.h
> new file mode 100644
> index 0000000000..f548ad2c9f
> --- /dev/null
> +++ b/SecurityPkg/Include/Library/RpmcLib.h
> @@ -0,0 +1,46 @@
> +/** @file
> 
> +  Public definitions for the Replay Protected Monotonic Counter (RPMC) Library.
> 
> +
> 
> +Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
> 
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> +
> 
> +**/
> 
> +
> 
> +#ifndef _RPMC_LIB_H_
> 
> +#define _RPMC_LIB_H_
> 
> +
> 
> +#include <Uefi/UefiBaseType.h>
> 
> +
> 
> +/**
> 
> +  Requests the current monotonic counter from the designated RPMC counter.
> 
> +
> 
> +  @param[in]    CounterId               Monotonic Counter Id.
> 
> +  @param[out]   CounterValue            A pointer to a buffer to store the RPMC
> value.
> 
> +
> 
> +  @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.
> 
> +**/
> 
> +EFI_STATUS
> 
> +EFIAPI
> 
> +RequestMonotonicCounter (
> 
> +  IN  UINT8   CounterId,
> 
> +  OUT UINT32  *CounterValue
> 
> +  );
> 
> +
> 
> +/**
> 
> +  Increments the designated monotonic counter in the SPI flash device by 1.
> 
> +
> 
> +  @param[in]    CounterId              Monotonic Counter Id.
> 
> +
> 
> +  @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.
> 
> +**/
> 
> +EFI_STATUS
> 
> +EFIAPI
> 
> +IncrementMonotonicCounter (
> 
> +  IN  UINT8   CounterId
> 
> +  );
> 
> +
> 
> +#endif
> \ No newline at end of file
> diff --git a/SecurityPkg/Include/Library/VariableKeyLib.h
> b/SecurityPkg/Include/Library/VariableKeyLib.h
> new file mode 100644
> index 0000000000..fe642b3d66
> --- /dev/null
> +++ b/SecurityPkg/Include/Library/VariableKeyLib.h
> @@ -0,0 +1,59 @@
> +/** @file
> 
> +  Public definitions for Variable Key Library.
> 
> +
> 
> +Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
> 
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> +
> 
> +**/
> 
> +
> 
> +#ifndef _VARIABLE_KEY_LIB_H_
> 
> +#define _VARIABLE_KEY_LIB_H_
> 
> +
> 
> +#include <Uefi/UefiBaseType.h>
> 
> +
> 
> +/**
> 
> +  Retrieves the variable root key.
> 
> +
> 
> +  @param[out]     VariableRootKey         A pointer to pointer for the variable
> root key buffer.
> 
> +  @param[in,out]  VariableRootKeySize     The size in bytes of the variable root
> key.
> 
> +
> 
> +  @retval       EFI_SUCCESS             The variable root key was returned.
> 
> +  @retval       EFI_DEVICE_ERROR        An error occurred while attempting to get
> the variable root key.
> 
> +  @retval       EFI_ACCESS_DENIED       The function was invoked after locking
> the key interface.
> 
> +  @retval       EFI_UNSUPPORTED         The variable root key is not supported in
> the current boot configuration.
> 
> +**/
> 
> +EFI_STATUS
> 
> +EFIAPI
> 
> +GetVariableRootKey (
> 
> +      OUT VOID    **VariableRootKey,
> 
> +  IN  OUT UINTN   *VariableRootKeySize
> 
> +  );
> 
> +
> 
> +/**
> 
> +  Regenerates the variable root key.
> 
> +
> 
> +  @retval       EFI_SUCCESS             The variable root key was regenerated
> successfully.
> 
> +  @retval       EFI_DEVICE_ERROR        An error occurred while attempting to
> regenerate the root key.
> 
> +  @retval       EFI_ACCESS_DENIED       The function was invoked after locking
> the key interface.
> 
> +  @retval       EFI_UNSUPPORTED         Key regeneration is not supported in the
> current boot configuration.
> 
> +**/
> 
> +EFI_STATUS
> 
> +EFIAPI
> 
> +RegenerateKey (
> 
> +  VOID
> 
> +  );
> 
> +
> 
> +/**
> 
> +  Locks the regenerate key interface.
> 
> +
> 
> +  @retval       EFI_SUCCESS             The key interface was locked successfully.
> 
> +  @retval       EFI_UNSUPPORTED         Locking the key interface is not supported
> in the current boot configuration.
> 
> +  @retval       Others                  An error occurred while attempting to lock the
> key interface.
> 
> +**/
> 
> +EFI_STATUS
> 
> +EFIAPI
> 
> +LockKeyInterface (
> 
> +  VOID
> 
> +  );
> 
> +
> 
> +#endif
> \ No newline at end of file
> diff --git a/SecurityPkg/SecurityPkg.dec b/SecurityPkg/SecurityPkg.dec
> index 5335cc5397..2cdfb02cc5 100644
> --- a/SecurityPkg/SecurityPkg.dec
> +++ b/SecurityPkg/SecurityPkg.dec
> @@ -76,6 +76,14 @@
>    #
> 
>    TcgStorageOpalLib|Include/Library/TcgStorageOpalLib.h
> 
> 
> 
> +  ## @libraryclass  Provides interfaces to access RPMC device.
> 
> +  #
> 
> +  RpmcLib|Include/Library/RpmcLib.h
> 
> +
> 
> +  ## @libraryclass  Provides interfaces to access variable root key.
> 
> +  #
> 
> +  VariableKeyLib|Include/Library/VariableKeyLib.h
> 
> +
> 
>  [Guids]
> 
>    ## Security package token space guid.
> 
>    # Include/Guid/SecurityPkgTokenSpace.h
> 
> --
> 2.24.0.windows.2