subject:"\[edk2\] \[PATCH V2 09\/50\] MdeModulePkg\/FmpAuthenticationLib\: Add FmpAuthenticationLib instance."

Re: [edk2] [PATCH V2 09/50] MdeModulePkg/FmpAuthenticationLib: Add FmpAuthenticationLib instance.

2016-10-11 Thread Yao, Jiewen

In ExecuteFmpAuthenticationHandler and other functions you use asserts to 
handle parameter validation.  I didn't see in the caller any protections 
against bad parameters so on builds with ASSERT disabled this code will not be 
safe.
[Jiewen] The code uses 4 ASSERT.
  ASSERT (Image != NULL); // This is filtered in FmpSetImage().
  ASSERT (ImageSize != 0); // I miss that check in FmpSetImage(). I will fix 
that.
  ASSERT (LastAttemptStatus != NULL); // This is an internal input. Caller may 
not able to control it.

  ASSERT (mNumberOfFmpAuthenticationHandler != 0); // This is an internal state 
check. Caller may not able to control it.

I already follow EDKII style to add comment on ASSERT() in function comment. So 
it is caller's responsibility to make sure they are valid.

Anyway, I think it is OK change ASSERT(Image) and ASSERT(ImageSize) to error 
handling, if you insist.
But LastAttempStatus is just internal input. Caller cannot control it. I do not 
see the need to use error handling.

Also mNumberOfFmpAuthenticationHandler is internal status. If it is 0, it means 
developer does not configure it successfully.
But the last issue will gone if we remove registration pattern.


Can you explain how you are using monotonic count?  Your comment says you are 
incrementing the PCD to avoid rollback (line 246: It is incremented during each 
firmware image operation.)
Looks like it is just being compared to make sure capsule counter not less than 
PCD based value?
[Jiewen] You are right. The purpose is to follow UEFI spec:
MonotonicCount It is included in the signature of AuthInfo. It is used to 
ensure freshness/no replay. It is incremented during each firmware image 
operation.

If I misunderstand something, please let me know.


Same comments as patch 3.  In my opinion library registration pattern is 
overkill for this usage.
[Jiewen] See my comment in previous mail.


From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Sean 
Brogan
Sent: Tuesday, October 11, 2016 5:51 PM
To: Yao, Jiewen <jiewen@intel.com>; edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Zeng, Star 
<star.z...@intel.com>; Tian, Feng <feng.t...@intel.com>; Gao, Liming 
<liming@intel.com>; Zhang, Chao B <chao.b.zh...@intel.com>
Subject: Re: [edk2] [PATCH V2 09/50] MdeModulePkg/FmpAuthenticationLib: Add 
FmpAuthenticationLib instance.

In ExecuteFmpAuthenticationHandler and other functions you use asserts to 
handle parameter validation.  I didn't see in the caller any protections 
against bad parameters so on builds with ASSERT disabled this code will not be 
safe.

Can you explain how you are using monotonic count?  Your comment says you are 
incrementing the PCD to avoid rollback (line 246: It is incremented during each 
firmware image operation.)
Looks like it is just being compared to make sure capsule counter not less than 
PCD based value?

Same comments as patch 3.  In my opinion library registration pattern is 
overkill for this usage.

Thanks
Sean



> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Jiewen Yao
> Sent: Friday, September 30, 2016 5:21 AM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Michael D Kinney 
> <michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>>; Feng Tian
> <feng.t...@intel.com<mailto:feng.t...@intel.com>>; Chao Zhang 
> <chao.b.zh...@intel.com<mailto:chao.b.zh...@intel.com>>; Liming Gao
> <liming@intel.com<mailto:liming....@intel.com>>; Star Zeng 
> <star.z...@intel.com<mailto:star.z...@intel.com>>
> Subject: [edk2] [PATCH V2 09/50] MdeModulePkg/FmpAuthenticationLib: Add
> FmpAuthenticationLib instance.
>
> This library is used to authenticate a UEFI defined FMP Capsule.
>
> Cc: Feng Tian <feng.t...@intel.com<mailto:feng.t...@intel.com>>
> Cc: Star Zeng <star.z...@intel.com<mailto:star.z...@intel.com>>
> Cc: Michael D Kinney 
> <michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>>
> Cc: Liming Gao <liming@intel.com<mailto:liming@intel.com>>
> Cc: Chao Zhang <chao.b.zh...@intel.com<mailto:chao.b.zh...@intel.com>>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen@intel.com<mailto:jiewen@intel.com>>
> Reviewed-by: Liming Gao <liming@intel.com<mailto:liming@intel.com>>
> ---
>  MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.c   | 277
> 
>  MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.inf |  47
>   MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.uni
> |  22 ++
>  3 files changed, 346 insertions(+)
>
> diff --git
> a/MdeModulePkg

Re: [edk2] [PATCH V2 09/50] MdeModulePkg/FmpAuthenticationLib: Add FmpAuthenticationLib instance.

2016-10-11 Thread Sean Brogan

In ExecuteFmpAuthenticationHandler and other functions you use asserts to 
handle parameter validation.  I didn't see in the caller any protections 
against bad parameters so on builds with ASSERT disabled this code will not be 
safe.  

Can you explain how you are using monotonic count?  Your comment says you are 
incrementing the PCD to avoid rollback (line 246: It is incremented during each 
firmware image operation.)
Looks like it is just being compared to make sure capsule counter not less than 
PCD based value?  

Same comments as patch 3.  In my opinion library registration pattern is 
overkill for this usage.  

Thanks
Sean
 


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Jiewen Yao
> Sent: Friday, September 30, 2016 5:21 AM
> To: edk2-devel@lists.01.org
> Cc: Michael D Kinney <michael.d.kin...@intel.com>; Feng Tian
> <feng.t...@intel.com>; Chao Zhang <chao.b.zh...@intel.com>; Liming Gao
> <liming@intel.com>; Star Zeng <star.z...@intel.com>
> Subject: [edk2] [PATCH V2 09/50] MdeModulePkg/FmpAuthenticationLib: Add
> FmpAuthenticationLib instance.
> 
> This library is used to authenticate a UEFI defined FMP Capsule.
> 
> Cc: Feng Tian <feng.t...@intel.com>
> Cc: Star Zeng <star.z...@intel.com>
> Cc: Michael D Kinney <michael.d.kin...@intel.com>
> Cc: Liming Gao <liming@intel.com>
> Cc: Chao Zhang <chao.b.zh...@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen@intel.com>
> Reviewed-by: Liming Gao <liming@intel.com>
> ---
>  MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.c   | 277
> 
>  MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.inf |  47
>   MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.uni
> |  22 ++
>  3 files changed, 346 insertions(+)
> 
> diff --git
> a/MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.c
> b/MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.c
> new file mode 100644
> index 000..878cbf9
> --- /dev/null
> +++ b/MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.c
> @@ -0,0 +1,277 @@
> +/** @file
> +  Provide generic FMP authentication functions for DXE/PEI post memory
> phase.
> +
> +  ExecuteFmpAuthenticationHandler() will receive untrusted input and do
> + basic  validation.
> +
> +  Copyright (c) 2016, Intel Corporation. All rights reserved.  This
> + program and the accompanying materials  are licensed and made
> + available under the terms and conditions of the BSD License  which
> + accompanies this distribution.  The full text of the license may be
> + found at  http://opensource.org/licenses/bsd-license.php.
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> BASIS,
> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include 
> +
> +#include 
> +#include 
> +#include  #include
> + #include  #include
> + #include 
> +
> +#define FMP_AUTHENTICATION_HANDLER_TABLE_SIZE   0x10
> +
> +UINT64   mMonotonicCount;
> +
> +UINT32   mNumberOfFmpAuthenticationHandler = 0;
> +UINT32   mMaxNumberOfFmpAuthenticationHandler = 0;
> +
> +GUID *mFmpAuthenticationHandlerGuidTable = NULL;
> +FMP_AUTHENTICATION_HANDLER   *mFmpAuthenticationHandlerTable =
> NULL;
> +
> +/**
> +  Reallocates more global memory to store the registered guid and Handler
> list.
> +
> +  @retval  RETURN_SUCCESSReallocated more global memory space to
> store guid and function tables.
> +  @retval  RETURN_OUT_OF_RESOURCES   Not enough memory to allocate.
> +**/
> +RETURN_STATUS
> +EFIAPI
> +ReallocateFmpAuthenticationHandlerTable (
> +  )
> +{
> +  //
> +  // Reallocate memory for GuidTable
> +  //
> +  mFmpAuthenticationHandlerGuidTable = ReallocatePool (
> + 
> mMaxNumberOfFmpAuthenticationHandler * sizeof
> (GUID),
> + 
> (mMaxNumberOfFmpAuthenticationHandler +
> FMP_AUTHENTICATION_HANDLER_TABLE_SIZE) * sizeof (GUID),
> + mFmpAuthenticationHandlerGuidTable
> + );
> +  if (mFmpAuthenticationHandlerGuidTable == NULL) {
> +goto Done;
> +  }
> +
> +  //
> +  // Reallocate memory for Authentication handler Table  //
> + mFmpAuthenticationHandlerTable = ReallocatePool (
> + mMaxNumberOfFmpAuthenticationHandler * 
> sizeof
> (FMP_AU

[edk2] [PATCH V2 09/50] MdeModulePkg/FmpAuthenticationLib: Add FmpAuthenticationLib instance.

2016-09-30 Thread Jiewen Yao

This library is used to authenticate a UEFI defined FMP Capsule.

Cc: Feng Tian 
Cc: Star Zeng 
Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Chao Zhang 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao 
Reviewed-by: Liming Gao 
---
 MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.c   | 277 

 MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.inf |  47 
 MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.uni |  22 ++
 3 files changed, 346 insertions(+)

diff --git a/MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.c 
b/MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.c
new file mode 100644
index 000..878cbf9
--- /dev/null
+++ b/MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.c
@@ -0,0 +1,277 @@
+/** @file
+  Provide generic FMP authentication functions for DXE/PEI post memory phase.
+
+  ExecuteFmpAuthenticationHandler() will receive untrusted input and do basic
+  validation.
+
+  Copyright (c) 2016, Intel Corporation. All rights reserved.
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of the BSD 
License
+  which accompanies this distribution.  The full text of the license may be 
found at
+  http://opensource.org/licenses/bsd-license.php.
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define FMP_AUTHENTICATION_HANDLER_TABLE_SIZE   0x10
+
+UINT64   mMonotonicCount;
+
+UINT32   mNumberOfFmpAuthenticationHandler = 0;
+UINT32   mMaxNumberOfFmpAuthenticationHandler = 0;
+
+GUID *mFmpAuthenticationHandlerGuidTable = NULL;
+FMP_AUTHENTICATION_HANDLER   *mFmpAuthenticationHandlerTable = NULL;
+
+/**
+  Reallocates more global memory to store the registered guid and Handler list.
+
+  @retval  RETURN_SUCCESSReallocated more global memory space to 
store guid and function tables.
+  @retval  RETURN_OUT_OF_RESOURCES   Not enough memory to allocate.
+**/
+RETURN_STATUS
+EFIAPI
+ReallocateFmpAuthenticationHandlerTable (
+  )
+{
+  //
+  // Reallocate memory for GuidTable
+  //
+  mFmpAuthenticationHandlerGuidTable = ReallocatePool (
+ mMaxNumberOfFmpAuthenticationHandler 
* sizeof (GUID),
+ (mMaxNumberOfFmpAuthenticationHandler 
+ FMP_AUTHENTICATION_HANDLER_TABLE_SIZE) * sizeof (GUID),
+ mFmpAuthenticationHandlerGuidTable
+ );
+  if (mFmpAuthenticationHandlerGuidTable == NULL) {
+goto Done;
+  }
+
+  //
+  // Reallocate memory for Authentication handler Table
+  //
+  mFmpAuthenticationHandlerTable = ReallocatePool (
+ mMaxNumberOfFmpAuthenticationHandler * 
sizeof (FMP_AUTHENTICATION_HANDLER),
+ (mMaxNumberOfFmpAuthenticationHandler + 
FMP_AUTHENTICATION_HANDLER_TABLE_SIZE) * sizeof (FMP_AUTHENTICATION_HANDLER),
+ mFmpAuthenticationHandlerTable
+ );
+  if (mFmpAuthenticationHandlerTable == NULL) {
+goto Done;
+  }
+
+  //
+  // Increase max handler number
+  //
+  mMaxNumberOfFmpAuthenticationHandler = mMaxNumberOfFmpAuthenticationHandler 
+ FMP_AUTHENTICATION_HANDLER_TABLE_SIZE;
+  return RETURN_SUCCESS;
+
+Done:
+  if (mFmpAuthenticationHandlerGuidTable != NULL) {
+FreePool (mFmpAuthenticationHandlerGuidTable);
+  }
+  if (mFmpAuthenticationHandlerTable != NULL) {
+FreePool (mFmpAuthenticationHandlerTable);
+  }
+
+  return RETURN_OUT_OF_RESOURCES;
+}
+
+/**
+  Constructor allocates the global memory to store the registered guid and 
Handler list.
+
+  @param  ImageHandle   The firmware allocated handle for the EFI image.
+  @param  SystemTable   A pointer to the EFI System Table.
+
+  @retval  RETURN_SUCCESSAllocated the global memory space to 
store guid and function tables.
+  @retval  RETURN_OUT_OF_RESOURCES   Not enough memory to allocate.
+**/
+RETURN_STATUS
+EFIAPI
+FmpAuthenticationLibConstructor (
+  VOID
+  )
+{
+  mMonotonicCount = PcdGet64(PcdEdkiiSystemFmpCapsuleMonotonicCount);
+
+  return ReallocateFmpAuthenticationHandlerTable ();
+}
+
+/**
+  Register FMP authentication handler with CertType.
+
+  If CertType is NULL, then ASSERT().
+  If FmpAuthenticationHandler is NULL, then ASSERT().
+
+  @param[in]  CertType   The certificate type associated with 
the FMP auth handler.
+  @param[in]  FmpAuthenticationHandler

Re: [edk2] [PATCH V2 09/50] MdeModulePkg/FmpAuthenticationLib: Add FmpAuthenticationLib instance.

Re: [edk2] [PATCH V2 09/50] MdeModulePkg/FmpAuthenticationLib: Add FmpAuthenticationLib instance.

[edk2] [PATCH V2 09/50] MdeModulePkg/FmpAuthenticationLib: Add FmpAuthenticationLib instance.

3 matches

Site Navigation

Mail list logo

Footer information