Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/UefiBootManagerLib: log reserved mem allocation failure

[Wu, Hao A]

> -Original Message-
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, January 09, 2020 7:43 AM
> To: edk2-devel-groups-io
> Cc: Wu, Hao A; Wang, Jian J; Ni, Ray; Gao, Zhichao
> Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/UefiBootManagerLib: log
> reserved mem allocation failure
> 
> The LoadFile protocol can report such a large buffer size that we cannot
> allocate enough reserved pages for. This particularly affects HTTP(S)
> Boot, if the remote file is very large (for example, an ISO image).
> 
> While the TianoCore wiki mentions this at
>  disk-image-size>:
> 
> > The maximum RAM disk image size depends on how much continuous
> reserved
> > memory block the platform could provide.
> 
> it's hard to remember; so log a DEBUG_ERROR message when the allocation
> fails.
> 
> This patch produces error messages such as:
> 
> > UiApp:BmExpandLoadFile: failed to allocate reserved pages:
> > BufferSize=4501536768
> > LoadFile="PciRoot(0x0)/Pci(0x3,0x0)/MAC(5254001B103E,0x1)/
> >  IPv4(0.0.0.0,TCP,DHCP,192.168.124.106,192.168.124.1,255.255.255.0)/
> >  Dns(192.168.124.1)/
> >  Uri(https://ipv4-server/RHEL-7.7-20190723.1-Server-x86_64-dvd1.iso)"
> > FilePath=""


Acked-by: Hao A Wu 

Best Regards,
Hao Wu


> 
> (Manually rewrapped here for keeping PatchCheck.py happy.)
> 
> Cc: Hao A Wu 
> Cc: Jian J Wang 
> Cc: Ray Ni 
> Cc: Zhichao Gao 
> Signed-off-by: Laszlo Ersek 
> ---
>  MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 31
> 
>  1 file changed, 31 insertions(+)
> 
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> index 62c5b2dc94ab..540d169ec1a6 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> @@ -1387,6 +1387,37 @@ BmExpandLoadFile (
>//
>FileBuffer = AllocateReservedPages (EFI_SIZE_TO_PAGES (BufferSize));
>if (FileBuffer == NULL) {
> +DEBUG_CODE (
> +  EFI_DEVICE_PATH *LoadFilePath;
> +  CHAR16  *LoadFileText;
> +  CHAR16  *FileText;
> +
> +  LoadFilePath = DevicePathFromHandle (LoadFileHandle);
> +  if (LoadFilePath == NULL) {
> +LoadFileText = NULL;
> +  } else {
> +LoadFileText = ConvertDevicePathToText (LoadFilePath, FALSE, FALSE);
> +  }
> +  FileText = ConvertDevicePathToText (FilePath, FALSE, FALSE);
> +
> +  DEBUG ((
> +DEBUG_ERROR,
> +"%a:%a: failed to allocate reserved pages: "
> +"BufferSize=%Lu LoadFile=\"%s\" FilePath=\"%s\"\n",
> +gEfiCallerBaseName,
> +__FUNCTION__,
> +(UINT64)BufferSize,
> +LoadFileText,
> +FileText
> +));
> +
> +  if (FileText != NULL) {
> +FreePool (FileText);
> +  }
> +  if (LoadFileText != NULL) {
> +FreePool (LoadFileText);
> +  }
> +  );
>  return NULL;
>}
> 
> --
> 2.19.1.3.g30247aa5d201
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53209): https://edk2.groups.io/g/devel/message/53209
Mute This Topic: https://groups.io/mt/69550077/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix VarErrorFlag RT cache offset calculation

[Wang, Jian J]

Michael,

I'm not sure sync-ing whole variable cache memory is an efficient operation.
What about using mVariableModuleGlobal->NonVolatileLastVariableOffset
as Length parameter?

   Status =  SynchronizeRuntimeVariableCache (
   
>VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
   0,
   mVariableModuleGlobal->NonVolatileLastVariableOffset
   );

Regards,
Jian

> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of Kubacki,
> Michael A
> Sent: Tuesday, January 14, 2020 7:19 AM
> To: devel@edk2.groups.io
> Cc: Gao, Liming ; Kinney, Michael D
> ; Michael Turner
> ; Wang, Jian J ; Wu,
> Hao A 
> Subject: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix VarErrorFlag
> RT cache offset calculation
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2457
> 
> This commit fixes an offset calculation that is used to write the
> VarErrorFlag UEFI variable to the UEFI variable runtime cache.
> 
> Currently a physical address is used instead of an offset. This
> commit changes the offset to zero with a length of the entire
> non-volatile variable store so the entire non-volatile variable
> store buffer in SMRAM (with the variable update modification) is
> copied to the runtime variable cache. This follows the same pattern
> used in other SynchronizeRuntimeVariableCache () calls for
> consistency.
> 
> * Observable symptom: An exception in SMM will most likely occur
>   due to the invalid memory reference when the VarErrorFlag variable
>   is written. The variable is most commonly written when the UEFI
>   variable store is full.
> 
> * The issue only occurs when the variable runtime cache is enabled
>   by the following PCD being set to TRUE:
>   gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache
> 
> Fixes: aab3b9b9a1e5e1f3fa966fb1667fc3e6c47e7706
> 
> Cc: Liming Gao 
> Cc: Michael D Kinney 
> Cc: Michael Turner 
> Cc: Jian J Wang 
> Cc: Hao A Wu 
> Signed-off-by: Michael Kubacki 
> ---
>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> index b0ee5e50d0..d23aea4bc7 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> @@ -16,7 +16,7 @@
>VariableServiceSetVariable() should also check authenticate data to avoid
> buffer overflow,
>integer overflow. It should also check attribute to avoid authentication 
> bypass.
> 
> -Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
> +Copyright (c) 2006 - 2020, Intel Corporation. All rights reserved.
>  (C) Copyright 2015-2018 Hewlett Packard Enterprise Development LP
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -335,8 +335,8 @@ RecordVarErrorFlag (
>*VarErrFlag = TempFlag;
>Status =  SynchronizeRuntimeVariableCache (
>
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
> -  (UINTN) VarErrFlag - (UINTN) mNvVariableCache + (UINTN)
> mVariableModuleGlobal->VariableGlobal.NonVolatileVariableBase,
> -  sizeof (TempFlag)
> +  0,
> +  mNvVariableCache->Size
>);
>ASSERT_EFI_ERROR (Status);
>  }
> --
> 2.16.2.windows.1
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53208): https://edk2.groups.io/g/devel/message/53208
Mute This Topic: https://groups.io/mt/69681408/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2] CryptoPkg/BaseCryptLib: remove HmacXxxGetContextSize interface

[Wang, Jian J]

> v2 changes:
> - change HmacXxxInit to HmacXxxSetKey
> - remove calling to HMAC_CTX_reset() since HmacXxxNew() has done it
>   already and user-supplied context is not supported any longer.
> - update comments affected by above change

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1792

Hmac(Md5|Sha1|Sha256)GetContextSize() use a deprecated macro
HMAC_MAX_MD_CBLOCK defined in openssl. They should be dropped to
avoid misuses in the future. For context allocation and release,
use HmacXxxNew() and HmacXxxFree() instead.

Considering that HmacXxxInit() has no use cases, remove the calling
to memset() and HMAC_CTX_reset() to drop the support of user supplied
context buffer. The only functionality left is to set user supplied
key for HMAC. To avoid confusion, change the the its name to
HmacXxxSetKey.

Cc: Xiaoyu Lu 
Cc: Laszlo Ersek 
Signed-off-by: Jian J Wang 
---
 CryptoPkg/Include/Library/BaseCryptLib.h  | 111 +-
 .../Library/BaseCryptLib/Hmac/CryptHmacMd5.c  |  58 ++---
 .../BaseCryptLib/Hmac/CryptHmacMd5Null.c  |  28 +
 .../Library/BaseCryptLib/Hmac/CryptHmacSha1.c |  59 ++
 .../BaseCryptLib/Hmac/CryptHmacSha1Null.c |  28 +
 .../BaseCryptLib/Hmac/CryptHmacSha256.c   |  58 ++---
 .../BaseCryptLib/Hmac/CryptHmacSha256Null.c   |  28 +
 .../BaseCryptLibNull/Hmac/CryptHmacMd5Null.c  |  20 
 .../BaseCryptLibNull/Hmac/CryptHmacSha1Null.c |  20 
 .../Hmac/CryptHmacSha256Null.c|  20 
 10 files changed, 72 insertions(+), 358 deletions(-)

diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h 
b/CryptoPkg/Include/Library/BaseCryptLib.h
index 8fe303a0b3..b93f1b9009 100644
--- a/CryptoPkg/Include/Library/BaseCryptLib.h
+++ b/CryptoPkg/Include/Library/BaseCryptLib.h
@@ -1025,23 +1025,6 @@ Sm3HashAll (
 //MAC (Message Authentication Code) Primitive
 
//=
 
-/**
-  Retrieves the size, in bytes, of the context buffer required for HMAC-MD5 
operations.
-  (NOTE: This API is deprecated.
- Use HmacMd5New() / HmacMd5Free() for HMAC-MD5 Context operations.)
-
-  If this interface is not supported, then return zero.
-
-  @return  The size, in bytes, of the context buffer required for HMAC-MD5 
operations.
-  @retval  0   This interface is not supported.
-
-**/
-UINTN
-EFIAPI
-HmacMd5GetContextSize (
-  VOID
-  );
-
 /**
   Allocates and initializes one HMAC_CTX context for subsequent HMAC-MD5 use.
 
@@ -1073,24 +1056,24 @@ HmacMd5Free (
   );
 
 /**
-  Initializes user-supplied memory pointed by HmacMd5Context as HMAC-MD5 
context for
-  subsequent use.
+  Set user-supplied key for subsequent use. It must be done before any
+  calling to HmacMd5Update().
 
   If HmacMd5Context is NULL, then return FALSE.
   If this interface is not supported, then return FALSE.
 
-  @param[out]  HmacMd5Context  Pointer to HMAC-MD5 context being initialized.
+  @param[out]  HmacMd5Context  Pointer to HMAC-MD5 context.
   @param[in]   Key Pointer to the user-supplied key.
   @param[in]   KeySize Key size in bytes.
 
-  @retval TRUE   HMAC-MD5 context initialization succeeded.
-  @retval FALSE  HMAC-MD5 context initialization failed.
+  @retval TRUE   Key is set succeefully.
+  @retval FALSE  Key is set unsucceefully.
   @retval FALSE  This interface is not supported.
 
 **/
 BOOLEAN
 EFIAPI
-HmacMd5Init (
+HmacMd5SetKey (
   OUT  VOID *HmacMd5Context,
   IN   CONST UINT8  *Key,
   IN   UINTNKeySize
@@ -1123,8 +1106,8 @@ HmacMd5Duplicate (
 
   This function performs HMAC-MD5 digest on a data buffer of the specified 
size.
   It can be called multiple times to compute the digest of long or 
discontinuous data streams.
-  HMAC-MD5 context should be already correctly initialized by HmacMd5Init(), 
and should not be
-  finalized by HmacMd5Final(). Behavior with invalid context is undefined.
+  HMAC-MD5 context should be initialized by HmacMd5New(), and should not be 
finalized by
+  HmacMd5Final(). Behavior with invalid context is undefined.
 
   If HmacMd5Context is NULL, then return FALSE.
   If this interface is not supported, then return FALSE.
@@ -1152,8 +1135,8 @@ HmacMd5Update (
   This function completes HMAC-MD5 hash computation and retrieves the digest 
value into
   the specified memory. After this function has been called, the HMAC-MD5 
context cannot
   be used again.
-  HMAC-MD5 context should be already correctly initialized by HmacMd5Init(), 
and should not be
-  finalized by HmacMd5Final(). Behavior with invalid HMAC-MD5 context is 
undefined.
+  HMAC-MD5 context should be initialized by HmacMd5New(), and should not be 
finalized by
+  HmacMd5Final(). Behavior with invalid HMAC-MD5 context is undefined.
 
   If HmacMd5Context is NULL, then return FALSE.
   If HmacValue is NULL, then return FALSE.
@@ -1175,23 +1158,6 @@ HmacMd5Final (
   OUT UINT8  *HmacValue
   );
 
-/**
-  Retrieves the size, in bytes, of 

Re: [edk2-devel] [PATCH] CryptoPkg/BaseCryptLib: deprecate HmacXxxGetContextSize interface

[Wang, Jian J]

Laszlo,

Good suggestions. I'll send v2 patch soon.

Regards,
Jian

> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of Laszlo Ersek
> Sent: Thursday, January 09, 2020 10:20 PM
> To: Wang, Jian J ; devel@edk2.groups.io
> Cc: Lu, XiaoyuX 
> Subject: Re: [edk2-devel] [PATCH] CryptoPkg/BaseCryptLib: deprecate
> HmacXxxGetContextSize interface
> 
> On 01/09/20 03:40, Wang, Jian J wrote:
> >> -Original Message-
> >> From: Laszlo Ersek 
> >> Sent: Wednesday, January 08, 2020 6:24 PM
> >> To: Wang, Jian J ; devel@edk2.groups.io
> >> Cc: Lu, XiaoyuX 
> >> Subject: Re: [PATCH] CryptoPkg/BaseCryptLib: deprecate
> >> HmacXxxGetContextSize interface
> >>
> >> On 01/08/20 08:26, Jian J Wang wrote:
> 
> >>> /**
> >>>   Initializes user-supplied memory pointed by HmacSha256Context as HMAC-
> >> SHA256 context for
> >>>   subsequent use.
> >>>
> >>>   If HmacSha256Context is NULL, then return FALSE.
> >>>
> >>>   @param[out]  HmacSha256Context  Pointer to HMAC-SHA256 context
> being
> >> initialized.
> >>>   @param[in]   KeyPointer to the user-supplied key.
> >>>   @param[in]   KeySizeKey size in bytes.
> >>>
> >>>   @retval TRUE   HMAC-SHA256 context initialization succeeded.
> >>>   @retval FALSE  HMAC-SHA256 context initialization failed.
> >>>
> >>> **/
> >>> BOOLEAN
> >>> EFIAPI
> >>> HmacSha256Init (
> >>>   OUT  VOID *HmacSha256Context,
> >>>   IN   CONST UINT8  *Key,
> >>>   IN   UINTNKeySize
> >>>   )
> >>> {
> >>>   //
> >>>   // Check input parameters.
> >>>   //
> >>>   if (HmacSha256Context == NULL || KeySize > INT_MAX) {
> >>> return FALSE;
> >>>   }
> >>>
> >>>   //
> >>>   // OpenSSL HMAC-SHA256 Context Initialization
> >>>   //
> >>>   memset(HmacSha256Context, 0, HMAC_SHA256_CTX_SIZE);
> >>>   if (HMAC_CTX_reset ((HMAC_CTX *)HmacSha256Context) != 1) {
> >>> return FALSE;
> >>>   }
> >>>   if (HMAC_Init_ex ((HMAC_CTX *)HmacSha256Context, Key, (UINT32)
> KeySize,
> >> EVP_sha256(), NULL) != 1) {
> >>> return FALSE;
> >>>   }
> >>>
> >>>   return TRUE;
> >>> }
> >>
> >> As the leading comment says, "HmacSha256Context" is user-supplied
> >> memory. If you remove the memset() call from the function, then
> >> HMAC_CTX_reset() will be invoked on user-supplied memory that may not
> >> have been cleared. Then HMAC_CTX_reset() will be called on garbage.
> >>
> >
> > You're right, if the user can supply a chunk of memory with *appropriate*
> > size as HmacContext. Since we deleted the macro HMAC_XXX_CTX_SIZE,
> > it's impossible for user to do that now. HMAC_CTX is a forward declaration.
> > MSVC refuses to give result of sizeof (HMAC_CTX). The user cannot know
> > how many bytes needed by HMAC_CTX. Therefore there's no such use cases
> > any longer. I think we could update the comments to enforce the use of
> > HmacXxxNew() to get context.  User supplied-memory is not acceptable.
> >
> > We can still keep the HMAC_CTX_reset line so that the user can still re-use
> > the context got before by HmacXxxNew(). I think HMAC_CTX_reset works
> > well with an empty Context or init-ed Context.
> >
> >> (2) The only way that I can see for fixing this problem is to remove the
> >> Hmac(Md5|Sha1|Sha256)Init functions too.
> >>
> >> I think that is safe to do, because I can't see any callers in the edk2
> >> codebase.
> >>
> >> One tricky part is that the leading comments of the
> >> Hmac(Md5|Sha1|Sha256)(Update|Final) functions refer to
> >> Hmac(Md5|Sha1|Sha256)Init. In other words, we do not have code
> >> references to Hmac(Md5|Sha1|Sha256)Init, but we have documentation
> >> references. This means that those comments should be updated as well --
> >> they should refer to Hmac(Md5|Sha1|Sha256)New instead.
> >>
> >
> > The Init interface is needed to supply user's key for HMAC. It seems the 
> > only
> > way to do that. I suggest to keep it.
> >
> >> (3) In case we'd like to continue providing functions that accept "Key"
> >> and "KeySize", for HMAC context initialization, then those functions
> >> will have to call HMAC_CTX_new() internally. Meaning that they can no
> >> longer take user-supplied memory; the context will have to be allocated
> >> inside OpenSSL, and returned to the caller.
> >
> > Yes, the variable encryption feature I'm working on needs to supply user
> > supplied key. I think it'd be better to keep it. Like I suggested above, we
> > should not allow user-supplied context and it's almost impossible for use
> > to supply correct size of context.
> 
> Assuming I understand your response correctly, I would suggest:
> 
> (1) Renaming "Init" to "SetKey",
> 
> (2) deleting both the memset() and the HMAC_CTX_reset() calls from "SetKey"
> 
> (3) updating the comment on "SetKey" so that it does not refer to "user
> supplied memory"; instead, it should say that "SetKey" can only be
> called on context returned by the appropriate "New" call, and only
> immediately after the "New" call (no intervening operations permitted on
> the 

Re: [edk2-devel] [PATCH 1/3] MdeModulePkg/SdMmcPciHcDxe: Refactor command error detection

[Wu, Hao A]

> -Original Message-
> From: Albecki, Mateusz
> Sent: Monday, January 13, 2020 9:49 PM
> To: Wu, Hao A; devel@edk2.groups.io
> Cc: Marcin Wojtas; Gao, Zhichao; Gao, Liming
> Subject: RE: [PATCH 1/3] MdeModulePkg/SdMmcPciHcDxe: Refactor
> command error detection
> 
> Hi,
> 
> I will fix the 2 issues in separate patch probably before doing the refactor 
> to
> avoid reverting it if the refactor introduces some unexpected issues.
> 
> Please also see inline.
> 
> Thanks,
> Mateusz
> 
> > -Original Message-
> > From: Wu, Hao A 
> > Sent: Friday, January 10, 2020 6:38 AM
> > To: Albecki, Mateusz ;
> devel@edk2.groups.io
> > Cc: Marcin Wojtas ; Gao, Zhichao
> > ; Gao, Liming 
> > Subject: RE: [PATCH 1/3] MdeModulePkg/SdMmcPciHcDxe: Refactor
> > command error detection
> >
> > Hello Mateusz,
> >
> > Some inline comments below:
> >
> >
> > > -Original Message-
> > > From: Albecki, Mateusz
> > > Sent: Tuesday, January 07, 2020 7:06 PM
> > > To: devel@edk2.groups.io
> > > Cc: Albecki, Mateusz; Wu, Hao A; Marcin Wojtas; Gao, Zhichao; Gao,
> > > Liming
> > > Subject: [PATCH 1/3] MdeModulePkg/SdMmcPciHcDxe: Refactor
> > command
> > > error detection
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1140
> > >
> > > Error detection function will now check if the command failure has
> > > been caused by one of the errors that can appear randomly on link(CRC
> > > error + end bit error). If such an error has been a cause of failure
> > > function will return EFI_CRC_ERROR instead of EFI_DEVICE_ERROR to
> > > indicate to the higher level that command has a chance of succeeding
> > > if resent. In addition this patch also fixes 2 small bugs. First one
> > > is DAT lane being reset on current limit error. Second one is data
> > > timeout error not being cleared after transfer has been completed.
> >
> >
> > For the 2 small issues, I would suggest to split them into separate patches,
> > which would make this patch into 3 patches. You can either do the
> > refactoring or fixing the 2 bugs first.
> >
> >
> > >
> > > Cc: Hao A Wu 
> > > Cc: Marcin Wojtas 
> > > Cc: Zhichao Gao 
> > > Cc: Liming Gao 
> > >
> > > Signed-off-by: Mateusz Albecki 
> > > ---
> > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 234
> > > +++
> > >  1 file changed, 158 insertions(+), 76 deletions(-)
> > >
> > > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > > index e7f2fac69b..8b5e54f321 100644
> > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > > @@ -7,7 +7,7 @@
> > >It would expose EFI_SD_MMC_PASS_THRU_PROTOCOL for upper layer
> > use.
> > >
> > >Copyright (c) 2018-2019, NVIDIA CORPORATION. All rights reserved.
> > > -  Copyright (c) 2015 - 2019, Intel Corporation. All rights
> > > reserved.
> > > +  Copyright (c) 2015 - 2020, Intel Corporation. All rights
> > > + reserved.
> > >SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > >  **/
> > > @@ -2137,6 +2137,154 @@ SdMmcExecTrb (
> > >return Status;
> > >  }
> > >
> > > +/**
> > > +  Performs SW reset based on passed error status mask.
> > > +
> > > +  @param[in]  Private   Pointer to driver private data.
> > > +  @param[in]  Slot  Index of the slot to reset.
> > > +  @param[in]  ErrIntStatus  Error interrupt status mask.
> > > +
> > > +  @retval EFI_SUCCESS  Software reset performed successfully.
> > > +  @retval OtherSoftware reset failed.
> > > +**/
> > > +EFI_STATUS
> > > +SdMmcSoftwareReset (
> > > +  IN SD_MMC_HC_PRIVATE_DATA  *Private,
> > > +  IN UINT8   Slot,
> > > +  IN UINT16  ErrIntStatus
> > > +  )
> > > +{
> > > +  UINT8   SwReset;
> > > +  EFI_STATUS  Status;
> > > +
> > > +  SwReset = 0;
> > > +  if ((ErrIntStatus & 0x0F) != 0) {
> > > +SwReset |= BIT1;
> > > +  }
> > > +  if ((ErrIntStatus & 0x70) != 0) {
> >
> >
> > Thanks for this catch.
> > Could you help to separate this fix to another patch?
> >
> >
> > > +SwReset |= BIT2;
> > > +  }
> > > +
> > > +  Status  = SdMmcHcRwMmio (
> > > +  Private->PciIo,
> > > +  Slot,
> > > +  SD_MMC_HC_SW_RST,
> > > +  FALSE,
> > > +  sizeof (SwReset),
> > > +  
> > > +  );
> > > +  if (EFI_ERROR (Status)) {
> > > +return Status;
> > > +  }
> > > +
> > > +  Status = SdMmcHcWaitMmioSet (
> > > + Private->PciIo,
> > > + Slot,
> > > + SD_MMC_HC_SW_RST,
> > > + sizeof (SwReset),
> > > + 0xFF,
> > > + 0,
> > > + SD_MMC_HC_GENERIC_TIMEOUT
> > > + );
> > > +  if (EFI_ERROR (Status)) {
> > > +return Status;
> > > +  }
> > > +
> > > +  return EFI_SUCCESS;
> > > +}
> > > +
> > > +/**
> > > +  Checks the error status in error status register
> > > +  and issues appropriate software 

Re: [edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles without device paths

[Ni, Ray]

Vitaly,
I still have concern to modify the EDKII code to workaround a firmware bug.
Can you just change in your local version?

Thanks,
Ray

From: devel@edk2.groups.io  On Behalf Of Vitaly Cheptsov 
via Groups.Io
Sent: Tuesday, January 14, 2020 4:47 AM
To: Laszlo Ersek ; devel@edk2.groups.io; Ni, Ray 
; Gao, Zhichao 
Subject: Re: [edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles without 
device paths


Thanks all for your input,



These explanations seem sufficient to us that it is not a good idea to change 
the behaviour for everyone. Even so, we still need this to be configurable in 
some way, as having to patch EDK II is impracticable.



We believe there are three possible routes to approach this problem:



  1.  Introduce a separate ControllerConnectionLib library for this function. 
While it is small, we found several places in our code that need to call it 
beyond UEFI Shell. This way different implementations could be used depending 
on the chosen library.
  2.  Introduce a ConnectRequiresDevicePath PCD, which will choose the 
preferred logic.
  3.  Introduce a -dp Shell argument for affected commands the way Lazslo 
suggested.



We believe either route or a combination of multiple routes have their own 
benefits, and would suggest either going with 1+2 or with 3. Any approach is 
fine for us.



We will submit V2 of the patch after hearing the opinions.



Best wishes,

Vitaly

On Mon, Jan 13, 2020 at 20:55, Laszlo Ersek 
mailto:ler...@redhat.com>> wrote:
On 01/13/20 12:56, Ni, Ray wrote:
> We shouldn't assume that a DriverBindingStart() can only start on a handle 
> with device path installed. DevicePath protocol is just a special protocol.
> It's possible that a bus driver starts on a host controller handle and 
> creates multiple children, each with only a Specific_IO protocol installed.
> Certain device driver can start on the children handle and open the 
> Specific_IO protocol BY_DRIVER.
> I am not sure if certain today's network drivers may work like this. It's 
> allowed per UEFI spec.

I agree.

Under "10.2 EFI Device Path Protocol", the spec says, "If the handle
does not logically map to a physical device, the handle may not
necessarily support the device path protocol."

I think gBS->ConnectController() and
EFI_DRIVER_BINDING_PROTOCOL.Supported() should work on such handles.

If we'd like to work around related issues in drivers, then I'd suggest
new command line options for the "load", "connect", "reconnect" shell
commands (maybe more), for filtering out handles that do not carry
device paths. Such command line options could be added as an extension,
IIUC, such as "-_option". I.e., I believe it's not necessary to start
with UEFI Shell Spec updates.

Thanks
Laszlo




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53204): https://edk2.groups.io/g/devel/message/53204
Mute This Topic: https://groups.io/mt/69653841/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix VarErrorFlag RT cache offset calculation

[Kubacki, Michael A]

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2457

This commit fixes an offset calculation that is used to write the
VarErrorFlag UEFI variable to the UEFI variable runtime cache.

Currently a physical address is used instead of an offset. This
commit changes the offset to zero with a length of the entire
non-volatile variable store so the entire non-volatile variable
store buffer in SMRAM (with the variable update modification) is
copied to the runtime variable cache. This follows the same pattern
used in other SynchronizeRuntimeVariableCache () calls for
consistency.

* Observable symptom: An exception in SMM will most likely occur
  due to the invalid memory reference when the VarErrorFlag variable
  is written. The variable is most commonly written when the UEFI
  variable store is full.

* The issue only occurs when the variable runtime cache is enabled
  by the following PCD being set to TRUE:
  gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache

Fixes: aab3b9b9a1e5e1f3fa966fb1667fc3e6c47e7706

Cc: Liming Gao 
Cc: Michael D Kinney 
Cc: Michael Turner 
Cc: Jian J Wang 
Cc: Hao A Wu 
Signed-off-by: Michael Kubacki 
---
 MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c 
b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index b0ee5e50d0..d23aea4bc7 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -16,7 +16,7 @@
   VariableServiceSetVariable() should also check authenticate data to avoid 
buffer overflow,
   integer overflow. It should also check attribute to avoid authentication 
bypass.
 
-Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
+Copyright (c) 2006 - 2020, Intel Corporation. All rights reserved.
 (C) Copyright 2015-2018 Hewlett Packard Enterprise Development LP
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -335,8 +335,8 @@ RecordVarErrorFlag (
   *VarErrFlag = TempFlag;
   Status =  SynchronizeRuntimeVariableCache (
   
>VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
-  (UINTN) VarErrFlag - (UINTN) mNvVariableCache + (UINTN) 
mVariableModuleGlobal->VariableGlobal.NonVolatileVariableBase,
-  sizeof (TempFlag)
+  0,
+  mNvVariableCache->Size
   );
   ASSERT_EFI_ERROR (Status);
 }
-- 
2.16.2.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53203): https://edk2.groups.io/g/devel/message/53203
Mute This Topic: https://groups.io/mt/69681408/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/UefiBootManagerLib: log reserved mem allocation failure

[Laszlo Ersek]

On 01/13/20 02:53, Siyuan, Fu wrote:
> The change looks good to me.
> 
> Reviewed-by: Siyuan Fu 

Thank you, Siyuan!

Hao, Jian, can one of you please give a formal Acked-by, based on
Siyuan's and Phil's "Reviewed-by"s? (I'm asking because I'm not sure if
you would be comfortable with me pushing this without MdeModulePkg "M"
approval.)

Thanks!
Laszlo

>> -Original Message-
>> From: devel@edk2.groups.io  On Behalf Of Laszlo
>> Ersek
>> Sent: 2020年1月9日 7:43
>> To: edk2-devel-groups-io 
>> Cc: Wu, Hao A ; Wang, Jian J ;
>> Ni, Ray ; Gao, Zhichao 
>> Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/UefiBootManagerLib: log
>> reserved mem allocation failure
>>
>> The LoadFile protocol can report such a large buffer size that we cannot
>> allocate enough reserved pages for. This particularly affects HTTP(S)
>> Boot, if the remote file is very large (for example, an ISO image).
>>
>> While the TianoCore wiki mentions this at
>> > disk-image-size>:
>>
>>> The maximum RAM disk image size depends on how much continuous
>> reserved
>>> memory block the platform could provide.
>>
>> it's hard to remember; so log a DEBUG_ERROR message when the allocation
>> fails.
>>
>> This patch produces error messages such as:
>>
>>> UiApp:BmExpandLoadFile: failed to allocate reserved pages:
>>> BufferSize=4501536768
>>> LoadFile="PciRoot(0x0)/Pci(0x3,0x0)/MAC(5254001B103E,0x1)/
>>>  IPv4(0.0.0.0,TCP,DHCP,192.168.124.106,192.168.124.1,255.255.255.0)/
>>>  Dns(192.168.124.1)/
>>>  Uri(https://ipv4-server/RHEL-7.7-20190723.1-Server-x86_64-dvd1.iso)"
>>> FilePath=""
>>
>> (Manually rewrapped here for keeping PatchCheck.py happy.)
>>
>> Cc: Hao A Wu 
>> Cc: Jian J Wang 
>> Cc: Ray Ni 
>> Cc: Zhichao Gao 
>> Signed-off-by: Laszlo Ersek 
>> ---
>>  MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 31
>> 
>>  1 file changed, 31 insertions(+)
>>
>> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>> b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>> index 62c5b2dc94ab..540d169ec1a6 100644
>> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>> @@ -1387,6 +1387,37 @@ BmExpandLoadFile (
>>//
>>FileBuffer = AllocateReservedPages (EFI_SIZE_TO_PAGES (BufferSize));
>>if (FileBuffer == NULL) {
>> +DEBUG_CODE (
>> +  EFI_DEVICE_PATH *LoadFilePath;
>> +  CHAR16  *LoadFileText;
>> +  CHAR16  *FileText;
>> +
>> +  LoadFilePath = DevicePathFromHandle (LoadFileHandle);
>> +  if (LoadFilePath == NULL) {
>> +LoadFileText = NULL;
>> +  } else {
>> +LoadFileText = ConvertDevicePathToText (LoadFilePath, FALSE, FALSE);
>> +  }
>> +  FileText = ConvertDevicePathToText (FilePath, FALSE, FALSE);
>> +
>> +  DEBUG ((
>> +DEBUG_ERROR,
>> +"%a:%a: failed to allocate reserved pages: "
>> +"BufferSize=%Lu LoadFile=\"%s\" FilePath=\"%s\"\n",
>> +gEfiCallerBaseName,
>> +__FUNCTION__,
>> +(UINT64)BufferSize,
>> +LoadFileText,
>> +FileText
>> +));
>> +
>> +  if (FileText != NULL) {
>> +FreePool (FileText);
>> +  }
>> +  if (LoadFileText != NULL) {
>> +FreePool (LoadFileText);
>> +  }
>> +  );
>>  return NULL;
>>}
>>
>> --
>> 2.19.1.3.g30247aa5d201
>>
>>
>>
>>
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53202): https://edk2.groups.io/g/devel/message/53202
Mute This Topic: https://groups.io/mt/69550077/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles without device paths

[Vitaly Cheptsov via Groups.Io]

Thanks all for your input,

These explanations seem sufficient to us that it is not a good idea to change 
the behaviour for everyone. Even so, we still need this to be configurable in 
some way, as having to patch EDK II is impracticable.

We believe there are three possible routes to approach this problem:

- Introduce a separate ControllerConnectionLib library for this function. While 
it is small, we found several places in our code that need to call it beyond 
UEFI Shell. This way different implementations could be used depending on the 
chosen library.
- Introduce a ConnectRequiresDevicePath PCD, which will choose the preferred 
logic.
- Introduce a -dp Shell argument for affected commands the way Lazslo suggested.

We believe either route or a combination of multiple routes have their own 
benefits, and would suggest either going with 1+2 or with 3. Any approach is 
fine for us.

We will submit V2 of the patch after hearing the opinions.

Best wishes,

Vitaly

On Mon, Jan 13, 2020 at 20:55, Laszlo Ersek  wrote:

> On 01/13/20 12:56, Ni, Ray wrote:
>> We shouldn't assume that a DriverBindingStart() can only start on a handle 
>> with device path installed. DevicePath protocol is just a special protocol.
>> It's possible that a bus driver starts on a host controller handle and 
>> creates multiple children, each with only a Specific_IO protocol installed.
>> Certain device driver can start on the children handle and open the 
>> Specific_IO protocol BY_DRIVER.
>> I am not sure if certain today's network drivers may work like this. It's 
>> allowed per UEFI spec.
>
> I agree.
>
> Under "10.2 EFI Device Path Protocol", the spec says, "If the handle
> does not logically map to a physical device, the handle may not
> necessarily support the device path protocol."
>
> I think gBS->ConnectController() and
> EFI_DRIVER_BINDING_PROTOCOL.Supported() should work on such handles.
>
> If we'd like to work around related issues in drivers, then I'd suggest
> new command line options for the "load", "connect", "reconnect" shell
> commands (maybe more), for filtering out handles that do not carry
> device paths. Such command line options could be added as an extension,
> IIUC, such as "-_option". I.e., I believe it's not necessary to start
> with UEFI Shell Spec updates.
>
> Thanks
> Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53201): https://edk2.groups.io/g/devel/message/53201
Mute This Topic: https://groups.io/mt/69653841/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 1/1] SecurityPkg: BaseHashLib: Implement Unified Hash API-for-

[Sukerkar, Amol N]

From: "Sukerkar, Amol N" 

This commit introduces a Unified Hash API to calculate hash using a
hashing algorithm specified by the PCD, PcdSystemHashPolicy. This library
interfaces with the various hashing API, such as, MD4, MD5, SHA1, SHA256,
SHA512 and SM3_256 implemented in CryptoPkg. The user can calculate the
desired hash by setting PcdSystemHashPolicy to appropriate value.

Cc: Jiewen Yao 
Cc: Jian J Wang 
Cc: Michael D Kinney 
Signed-off-by: Sukerkar, Amol N 
---
 SecurityPkg/Library/BaseHashLib/BaseHashLibCommon.c | 252 
 SecurityPkg/Library/BaseHashLib/BaseHashLibDxe.c| 122 ++
 SecurityPkg/Library/BaseHashLib/BaseHashLibPei.c| 125 ++
 SecurityPkg/Include/Library/BaseHashLib.h   |  84 +++
 SecurityPkg/Library/BaseHashLib/BaseHashLibCommon.h |  71 ++
 SecurityPkg/Library/BaseHashLib/BaseHashLibDxe.inf  |  47 
 SecurityPkg/Library/BaseHashLib/BaseHashLibDxe.uni  |  18 ++
 SecurityPkg/Library/BaseHashLib/BaseHashLibPei.inf  |  52 
 SecurityPkg/Library/BaseHashLib/BaseHashLibPei.uni  |  17 ++
 SecurityPkg/SecurityPkg.dec |  23 +-
 SecurityPkg/SecurityPkg.dsc |  10 +-
 SecurityPkg/SecurityPkg.uni |  15 +-
 12 files changed, 833 insertions(+), 3 deletions(-)

diff --git a/SecurityPkg/Library/BaseHashLib/BaseHashLibCommon.c 
b/SecurityPkg/Library/BaseHashLib/BaseHashLibCommon.c
new file mode 100644
index ..f8742e55b5f7
--- /dev/null
+++ b/SecurityPkg/Library/BaseHashLib/BaseHashLibCommon.c
@@ -0,0 +1,252 @@
+/** @file
+  Implement image verification services for secure boot service
+
+  Caution: This file requires additional review when modified.
+  This library will have external input - PE/COFF image.
+  This external input must be validated carefully to avoid security issue like
+  buffer overflow, integer overflow.
+
+  DxeImageVerificationLibImageRead() function will make sure the PE/COFF image 
content
+  read is within the image buffer.
+
+  DxeImageVerificationHandler(), HashPeImageByType(), HashPeImage() function 
will accept
+  untrusted PE/COFF image and validate its data structure within this image 
buffer before use.
+
+Copyright (c) 2009 - 2020, Intel Corporation. All rights reserved.
+(C) Copyright 2016 Hewlett Packard Enterprise Development LP
+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 
+
+/**
+  Init hash sequence with Hash Algorithm specified by HashPolicy.
+
+  @param HashPolicy  Hash Algorithm Policy.
+  @param HashHandle  Hash handle.
+
+  @retval TRUE   Hash start and HashHandle returned.
+  @retval FALSE  Hash Init unsuccessful.
+**/
+BOOLEAN
+EFIAPI
+HashInitInternal (
+  IN UINT8  HashPolicy, 
+  OUT HASH_HANDLE   *HashHandle
+  )
+{
+  BOOLEAN  Status;
+  VOID *HashCtx;
+  UINTNCtxSize;
+
+  switch (HashPolicy) {
+case HASH_MD4:
+  CtxSize = Md4GetContextSize ();
+  HashCtx = AllocatePool (CtxSize);
+  ASSERT (HashCtx != NULL);
+
+  Status = Md4Init (HashCtx);
+  break;
+
+case HASH_MD5:
+  CtxSize = Md5GetContextSize ();
+  HashCtx = AllocatePool (CtxSize);
+  ASSERT (HashCtx != NULL);
+
+ Status = Md5Init (HashCtx);
+  break;
+
+case HASH_SHA1:
+  CtxSize = Sha1GetContextSize ();
+  HashCtx = AllocatePool (CtxSize);
+  ASSERT (HashCtx != NULL);
+
+  Status = Sha1Init (HashCtx);
+  break;
+
+case HASH_SHA256:
+  CtxSize = Sha256GetContextSize ();
+  HashCtx = AllocatePool (CtxSize);
+  ASSERT (HashCtx != NULL);
+
+  Status = Sha256Init (HashCtx);
+  break;
+
+case HASH_SHA384:
+  CtxSize = Sha384GetContextSize ();
+  HashCtx = AllocatePool (CtxSize);
+  ASSERT (HashCtx != NULL);
+
+  Status = Sha384Init (HashCtx);
+  break;
+
+case HASH_SHA512:
+  CtxSize = Sha512GetContextSize ();
+  HashCtx = AllocatePool (CtxSize);
+  ASSERT (HashCtx != NULL);
+
+  Status = Sha512Init (HashCtx);
+  break;
+
+case HASH_SM3_256:
+  CtxSize = Sm3GetContextSize ();
+  HashCtx = AllocatePool (CtxSize);
+  ASSERT (HashCtx != NULL);
+
+  Status = Sm3Init (HashCtx);
+  break;
+
+default:
+  ASSERT (FALSE);
+  break;
+  }
+
+  *HashHandle = (HASH_HANDLE)HashCtx;
+
+  return Status;
+}
+
+/**
+  Update hash data with Hash Algorithm specified by HashPolicy.
+
+  @param HashPolicyHash Algorithm Policy.
+  @param HashHandleHash handle.
+  @param DataToHashData to be hashed.
+  @param 

[edk2-devel] [PATCH v1 0/1] *** Unified Hash Calucation API ***

[Sukerkar, Amol N]

From: "Sukerkar, Amol N" 

Currently the UEFI drivers using the SHA/SM3 hashing algorithms use hard-coded 
API to calculate the hash, such as, sha_256(…), etc. Since SHA384 and/or SM3 
are being increasingly adopted, it becomes cumbersome to modify the driver with 
SHA384 or SM3 calls for each application.

To better achieve this, we are proposing a unified API which can be used by 
UEFI drivers that provides the drivers with flexibility to use the hashing 
algorithm they desired or the strongest hashing algorithm the system supports 
(with openssl). Attached is the design proposal for the same and we request 
feedback from the community before we begin the process of making the changes 
to EDK2 repo.

Alternatively, the design document is also attached to Bugzilla, 
https://bugzilla.tianocore.org/show_bug.cgi?id=2151. You can also provide the 
feedback in the Bugzilla.


Sukerkar, Amol N (1):
  SecurityPkg: BaseHashLib: Implement Unified Hash API-for-

 SecurityPkg/Library/BaseHashLib/BaseHashLibCommon.c | 252 
 SecurityPkg/Library/BaseHashLib/BaseHashLibDxe.c| 122 ++
 SecurityPkg/Library/BaseHashLib/BaseHashLibPei.c| 125 ++
 SecurityPkg/Include/Library/BaseHashLib.h   |  84 +++
 SecurityPkg/Library/BaseHashLib/BaseHashLibCommon.h |  71 ++
 SecurityPkg/Library/BaseHashLib/BaseHashLibDxe.inf  |  47 
 SecurityPkg/Library/BaseHashLib/BaseHashLibDxe.uni  |  18 ++
 SecurityPkg/Library/BaseHashLib/BaseHashLibPei.inf  |  52 
 SecurityPkg/Library/BaseHashLib/BaseHashLibPei.uni  |  17 ++
 SecurityPkg/SecurityPkg.dec |  23 +-
 SecurityPkg/SecurityPkg.dsc |  10 +-
 SecurityPkg/SecurityPkg.uni |  15 +-
 12 files changed, 833 insertions(+), 3 deletions(-)
 create mode 100644 SecurityPkg/Library/BaseHashLib/BaseHashLibCommon.c
 create mode 100644 SecurityPkg/Library/BaseHashLib/BaseHashLibDxe.c
 create mode 100644 SecurityPkg/Library/BaseHashLib/BaseHashLibPei.c
 create mode 100644 SecurityPkg/Include/Library/BaseHashLib.h
 create mode 100644 SecurityPkg/Library/BaseHashLib/BaseHashLibCommon.h
 create mode 100644 SecurityPkg/Library/BaseHashLib/BaseHashLibDxe.inf
 create mode 100644 SecurityPkg/Library/BaseHashLib/BaseHashLibDxe.uni
 create mode 100644 SecurityPkg/Library/BaseHashLib/BaseHashLibPei.inf
 create mode 100644 SecurityPkg/Library/BaseHashLib/BaseHashLibPei.uni

-- 
2.16.2.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53199): https://edk2.groups.io/g/devel/message/53199
Mute This Topic: https://groups.io/mt/69676704/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch] BaseTools/Scripts/PatchCheck: Address false error conditions

[Michael D Kinney]

Hi Laszlo,

This patch has been pushed.

https://github.com/tianocore/edk2/pull/297

I also entered a new BZ to relax CVE pattern check.

https://bugzilla.tianocore.org/show_bug.cgi?id=2462

Mike

> -Original Message-
> From: devel@edk2.groups.io  On
> Behalf Of Laszlo Ersek
> Sent: Monday, January 13, 2020 9:32 AM
> To: Kinney, Michael D ;
> devel@edk2.groups.io
> Cc: Feng, Bob C ; Gao, Liming
> ; Justen, Jordan L
> 
> Subject: Re: [edk2-devel] [Patch]
> BaseTools/Scripts/PatchCheck: Address false error
> conditions
> 
> Hello Mike,
> 
> thanks a lot for this patch!
> 
> Comments below:
> 
> On 01/10/20 23:53, Michael D Kinney wrote:
> > https://bugzilla.tianocore.org/show_bug.cgi?id=2406
> >
> > * Always print subject line after the git commit id
> to make
> >   it easier to know the context of warnings or
> errors.
> > * Allow UTF-8 characters in subject line
> > * Error if subject line length > 75 without CVE-xxx-
> x present
> > * Error if subject line length > 92 with CVE--
> x present
> > * If body line length is > 75, then print warning
> instead of error.
> >
> > Cc: Bob Feng 
> > Cc: Liming Gao 
> > Cc: Laszlo Ersek 
> > Cc: Jordan Justen 
> > Signed-off-by: Michael D Kinney
> 
> > ---
> >  BaseTools/Scripts/PatchCheck.py | 57
> +
> >  1 file changed, 51 insertions(+), 6 deletions(-)
> >
> > diff --git a/BaseTools/Scripts/PatchCheck.py
> b/BaseTools/Scripts/PatchCheck.py
> > index 9668025798..ff4572bb7c 100755
> > --- a/BaseTools/Scripts/PatchCheck.py
> > +++ b/BaseTools/Scripts/PatchCheck.py
> > @@ -1,7 +1,7 @@
> >  ## @file
> >  #  Check a patch for various format issues
> >  #
> > -#  Copyright (c) 2015 - 2019, Intel Corporation. All
> rights reserved.
> > +#  Copyright (c) 2015 - 2020, Intel Corporation. All
> rights reserved.
> >  #
> >  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> >  #
> > @@ -35,6 +35,8 @@ class CommitMessageCheck:
> >  self.subject = subject
> >  self.msg = message
> >
> > +print (subject)
> > +
> >  self.check_contributed_under()
> >  self.check_signed_off_by()
> >  self.check_misc_signatures()
> > @@ -179,6 +181,8 @@ class CommitMessageCheck:
> >  for sig in self.sig_types:
> >  self.find_signatures(sig)
> >
> > +cve_re = re.compile('CVE-[0-9]{4}-[0-9]{5}[^0-
> 9]')
> > +
> 
> (1) I suggest we permit CVE IDs with 4 arbitrary digits
> too. That is:
> 
>   CVE-[0-9]{4}-[0-9]{4,5}[^0-9]
> ^
> 
> Because of:
> 
> 
> https://cve.mitre.org/cve/identifiers/syntaxchange.html
> #new
> 
> For example, "CVE-2020-0001" is an existent CVE:
> 
>   https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-
> 2020-0001
> 
> and edk2 could get a CVE assigned for which only 4
> "arbitrary digits"
> were necessary. (In that case, the CNA would not pad
> that sequence to 5
> digits.)
> 
> Now, I wouldn't like to overcomplicate this, therefore
> I'm not
> requesting that in such cases, we also lower the
> inclusive limit from 92
> to 91 characters, in the subject line.
> 
> (I think that this suggested update to the script/patch
> does not require
> a reposting. Another comment below.)
> 
> 
> >  def check_overall_format(self):
> >  lines = self.msg.splitlines()
> >
> > @@ -196,9 +200,26 @@ class CommitMessageCheck:
> >  self.error('Empty commit message!')
> >  return
> >
> > -if count >= 1 and len(lines[0].rstrip()) >=
> 72:
> > -self.error('First line of commit message
> (subject line) ' +
> > -   'is too long.')
> > +if count >= 1 and re.search(self.cve_re,
> lines[0]):
> > +#
> > +# If CVE--x is present in
> subject line, then limit length of
> > +# subject line to 92 characters
> > +#
> > +if len(lines[0].rstrip()) >= 93:
> > +self.error(
> > +'First line of commit message
> (subject line) is too long (%d >= 93).' %
> > +(len(lines[0].rstrip()))
> > +)
> > +else:
> > +#
> > +# If CVE--x is not present in
> subject line, then limit
> > +# length of subject line to 75
> characters
> > +#
> > +if len(lines[0].rstrip()) >= 76:
> > +self.error(
> > +'First line of commit message
> (subject line) is too long (%d >= 76).' %
> > +(len(lines[0].rstrip()))
> > +)
> >
> >  if count >= 1 and len(lines[0].strip()) ==
> 0:
> >  self.error('First line of commit message
> (subject line) ' +
> > @@ -212,7 +233,14 @@ class CommitMessageCheck:
> >  if (len(lines[i]) >= 76 and
> >  len(lines[i].split()) > 1 and
> >  not lines[i].startswith('git-svn-
> id:')):
> > -self.error('Line %d of commit
> 

Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####

[Andrew Fish via Groups.Io]

Ard,

Is the problem GFX console? Would it be possible to have a PCD to assume 
graphics console, and if non was found on the boot connect those PCI devices 
and update the NVRAM to cause a console to connect. You might have to do a 2nd 
connect on the GOP handle after the nvram variable was written to make the 
ConSpliter see it?

Thanks,

Andrew Fish

> On Jan 13, 2020, at 9:28 AM, Ard Biesheuvel  wrote:
> 
> On Fri, 10 Jan 2020 at 17:23, Laszlo Ersek  > wrote:
>> 
>> On 01/10/20 15:37, Ni, Ray wrote:
>>> Ard,
>>> I understand now that: BeforeConsole() needs to connect Gfx to get the GOP 
>>> device path
>>> for ConOut updating. But the GOP driver is specified in Driver and it 
>>> will be loaded after
>>> BeforeConsole() in today's code. This order makes the Gfx connection fails 
>>> to start the GOP driver
>>> resulting ConOut not updated.
>>> 
>>> Moving Driver processing before BeforeConsole() helps in your case. But 
>>> it may impact
>>> other platforms: There might be platforms that update Driver variables 
>>> in BeforeConsole()
>>> and expect these drivers are processed in the same boot (not the next 
>>> boot). I don't have the real
>>> examples. But today's flow allows this. With your change, Driver will 
>>> not be processed in the first
>>> boot. The change impacts these platforms.
>>> 
>>> One backward compatible approach can be: processing Driver before 
>>> BeforeConsole() and processing the new Driver (added by 
>>> BeforeConsole()) after BeforeConsole().
>>> 
>>> But another problem arises: If BeforeConsole() removes a Driver, 
>>> platform's expectation is that deleted Driver doesn't run. But that 
>>> driver already runs.
>>> 
>>> So actually the question is: when BDS core can consume the Driver and 
>>> process them?
>>> Right now, it’s the time after BeforeConsole(). Just as right now 
>>> BeforeConsole() updates ConOut
>>> in your case, BeforeConsole() is the place where platform updates all UEFI 
>>> defined boot related
>>> variables. Processing Driver before BeforeConsole() requires platform 
>>> updates Driver
>>> in other places. It's a new requirement to the platform.
>>> 
>>> With all what I explained in above, I cannot agree with the changes.
>>> 
>>> Another approach is:
>>> Platform could connect the GFX in AfterConsole() and update the ConOut. 
>>> Then platform could manually call EfiBootManagerConnectConsoleVariable 
>>> (ConOut) to re-connect ConOut.
>>> It's a bit ugly I agree.
>> 
>> Let me raise three other ideas (alternatives to each other, and to the 
>> above), with varying levels of annoyance. :)
>> 
> 
> Thanks Laszlo
> 
> Ray, given your objection to my approach, could you please consider
> the below and give feedback on which approach is suitable to address
> the issue I am trying to fix?
> 
>> 
>> (1) Keep the following logic (i.e. the subject of this patch):
>> 
>>  //
>>  // Execute Driver Options
>>  //
>>  LoadOptions = EfiBootManagerGetLoadOptions (, 
>> LoadOptionTypeDriver);
>>  ProcessLoadOptions (LoadOptions, LoadOptionCount);
>>  EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
>> 
>> in *both* places, but gate each one with a bit in a new bitmask PCD.
>> 
>> (Note: it's probably not the best for any platform to permit both branches, 
>> as driver images would be loaded twice.)
>> 
>> 
>> (2) EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL has recently been added to edk2. It 
>> looks like a well-designed (extensible) protocol, for two reasons:
>> 
>> - the protocol structure has a Revision field,
>> 
>> - the only current member function, RefreshAllBootOptions(), is permitted to 
>> return EFI_UNSUPPORTED -- and the single call site, in the 
>> EfiBootManagerRefreshAllBootOption() function, handles that return value 
>> well (it does nothing).
>> 
>> The idea would be to bump the Revision field, and add a new member function. 
>> Then call this member function (if it exists) in the spot where the current 
>> patch proposes to move the Driver dispatch logic to.
>> 
>> This is almost like a new PlatformBootManagerLib interface, except it does 
>> not require existent lib instances to be updated.
>> 
>> And, on the EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL implementation side, 
>> RefreshAllBootOptions() would return EFI_UNSUPPORTED. (Because that is 
>> irrelevant to Ard's use case.)
>> 
>> Perhaps add yet another member function that can disable the Driver 
>> option processing in the current location.
>> 
>> 
>> (3) Extend the UEFI specification, section "3.1.3 Load Options".
>> 
>> The LOAD_OPTION_CATEGORY bit-field is almost what we want, except it's 
>> specified to be ignored for Driver options. So,
>> 
>> - either invent the standardese to let us use LOAD_OPTION_CATEGORY for 
>> Driver options too, without breaking existing platforms, *or*
>> - introduce a new (not too wide) distinct bitfield called 
>> LOAD_OPTION_DRIVER_CATEGORY.
>> 
>> Whichever category 

Re: [edk2-devel] [PATCH] MdePkg: PE loader should zero out dest buffer on allocation

[Laszlo Ersek]

On 01/13/20 09:18, Zhiguang Liu wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1999
> 
> When PE loader loads image to memory, the first section of image may
> not locate right next to the image header, which causes some memory
> space remaining uninitialized. This is a security issue.
> This patch compares the ending address of image header and the beginning
> address of the first section. If there is a gap, zero out this gap.
> 
> Cc: Jian J Wang 
> Cc: Hao A Wu 
> Signed-off-by: Zhiguang Liu 
> ---
>  MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c 
> b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> index 07bb62f860..2cdfb4a082 100644
> --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> @@ -1306,6 +1306,14 @@ PeCoffLoaderLoadImage (
>// Load each section of the image
>//
>Section = FirstSection;
> +  //
> +  // Zero out the memory space between image header and the first section
> +  //
> +  End  = (CHAR8 *)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders);
> +  Base = PeCoffLoaderImageAddress (ImageContext, Section->VirtualAddress, 
> TeStrippedOffset);
> +  if (End < Base) {
> +ZeroMem (End, Base - End);
> +  }
>for (Index = 0; Index < NumberOfSections; Index++) {
>  //
>  // Read the section
> 

CC'ing Marvin

Thanks
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53196): https://edk2.groups.io/g/devel/message/53196
Mute This Topic: https://groups.io/mt/69664451/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles without device paths

[Laszlo Ersek]

On 01/13/20 12:56, Ni, Ray wrote:
> We shouldn't assume that a DriverBindingStart() can only start on a handle 
> with device path installed. DevicePath protocol is just a special protocol.
> It's possible that a bus driver starts on a host controller handle and 
> creates multiple children, each with only a Specific_IO protocol installed.
> Certain device driver can start on the children handle and open the 
> Specific_IO protocol BY_DRIVER.
> I am not sure if certain today's network drivers may work like this. It's 
> allowed per UEFI spec.

I agree.

Under "10.2 EFI Device Path Protocol", the spec says, "If the handle
does not logically map to a physical device, the handle may not
necessarily support the device path protocol."

I think gBS->ConnectController() and
EFI_DRIVER_BINDING_PROTOCOL.Supported() should work on such handles.

If we'd like to work around related issues in drivers, then I'd suggest
new command line options for the "load", "connect", "reconnect" shell
commands (maybe more), for filtering out handles that do not carry
device paths. Such command line options could be added as an extension,
IIUC, such as "-_option". I.e., I believe it's not necessary to start
with UEFI Shell Spec updates.

Thanks
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53194): https://edk2.groups.io/g/devel/message/53194
Mute This Topic: https://groups.io/mt/69653841/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch] BaseTools/Scripts/PatchCheck: Address false error conditions

[Laszlo Ersek]

Hello Mike,

thanks a lot for this patch!

Comments below:

On 01/10/20 23:53, Michael D Kinney wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=2406
>
> * Always print subject line after the git commit id to make
>   it easier to know the context of warnings or errors.
> * Allow UTF-8 characters in subject line
> * Error if subject line length > 75 without CVE-xxx-x present
> * Error if subject line length > 92 with CVE--x present
> * If body line length is > 75, then print warning instead of error.
>
> Cc: Bob Feng 
> Cc: Liming Gao 
> Cc: Laszlo Ersek 
> Cc: Jordan Justen 
> Signed-off-by: Michael D Kinney 
> ---
>  BaseTools/Scripts/PatchCheck.py | 57 +
>  1 file changed, 51 insertions(+), 6 deletions(-)
>
> diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
> index 9668025798..ff4572bb7c 100755
> --- a/BaseTools/Scripts/PatchCheck.py
> +++ b/BaseTools/Scripts/PatchCheck.py
> @@ -1,7 +1,7 @@
>  ## @file
>  #  Check a patch for various format issues
>  #
> -#  Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.
> +#  Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
> @@ -35,6 +35,8 @@ class CommitMessageCheck:
>  self.subject = subject
>  self.msg = message
>
> +print (subject)
> +
>  self.check_contributed_under()
>  self.check_signed_off_by()
>  self.check_misc_signatures()
> @@ -179,6 +181,8 @@ class CommitMessageCheck:
>  for sig in self.sig_types:
>  self.find_signatures(sig)
>
> +cve_re = re.compile('CVE-[0-9]{4}-[0-9]{5}[^0-9]')
> +

(1) I suggest we permit CVE IDs with 4 arbitrary digits too. That is:

  CVE-[0-9]{4}-[0-9]{4,5}[^0-9]
^

Because of:

  https://cve.mitre.org/cve/identifiers/syntaxchange.html#new

For example, "CVE-2020-0001" is an existent CVE:

  https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-0001

and edk2 could get a CVE assigned for which only 4 "arbitrary digits"
were necessary. (In that case, the CNA would not pad that sequence to 5
digits.)

Now, I wouldn't like to overcomplicate this, therefore I'm not
requesting that in such cases, we also lower the inclusive limit from 92
to 91 characters, in the subject line.

(I think that this suggested update to the script/patch does not require
a reposting. Another comment below.)


>  def check_overall_format(self):
>  lines = self.msg.splitlines()
>
> @@ -196,9 +200,26 @@ class CommitMessageCheck:
>  self.error('Empty commit message!')
>  return
>
> -if count >= 1 and len(lines[0].rstrip()) >= 72:
> -self.error('First line of commit message (subject line) ' +
> -   'is too long.')
> +if count >= 1 and re.search(self.cve_re, lines[0]):
> +#
> +# If CVE--x is present in subject line, then limit 
> length of
> +# subject line to 92 characters
> +#
> +if len(lines[0].rstrip()) >= 93:
> +self.error(
> +'First line of commit message (subject line) is too long 
> (%d >= 93).' %
> +(len(lines[0].rstrip()))
> +)
> +else:
> +#
> +# If CVE--x is not present in subject line, then limit
> +# length of subject line to 75 characters
> +#
> +if len(lines[0].rstrip()) >= 76:
> +self.error(
> +'First line of commit message (subject line) is too long 
> (%d >= 76).' %
> +(len(lines[0].rstrip()))
> +)
>
>  if count >= 1 and len(lines[0].strip()) == 0:
>  self.error('First line of commit message (subject line) ' +
> @@ -212,7 +233,14 @@ class CommitMessageCheck:
>  if (len(lines[i]) >= 76 and
>  len(lines[i].split()) > 1 and
>  not lines[i].startswith('git-svn-id:')):
> -self.error('Line %d of commit message is too long.' % (i + 
> 1))
> +#
> +# Print a warning if body line is longer than 75 characters
> +#
> +print(
> +'WARNING - Line %d of commit message is too long (%d >= 
> 76).' %
> +(i + 1, len(lines[i]))
> +)
> +print(lines[i])
>
>  last_sig_line = None
>  for i in range(count - 1, 0, -1):
> @@ -503,8 +531,25 @@ class CheckOnePatch:
>  else:
>  self.stat = mo.group('stat')
>  self.commit_msg = mo.group('commit_message')
> +#
> +# Parse subject line from email header.  The subject line may be
> +# composed of multiple parts with different encodings.  Decode and
> +# combine all the parts to produce a 

Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####

[Ard Biesheuvel]

On Fri, 10 Jan 2020 at 17:23, Laszlo Ersek  wrote:
>
> On 01/10/20 15:37, Ni, Ray wrote:
> > Ard,
> > I understand now that: BeforeConsole() needs to connect Gfx to get the GOP 
> > device path
> > for ConOut updating. But the GOP driver is specified in Driver and it 
> > will be loaded after
> > BeforeConsole() in today's code. This order makes the Gfx connection fails 
> > to start the GOP driver
> > resulting ConOut not updated.
> >
> > Moving Driver processing before BeforeConsole() helps in your case. But 
> > it may impact
> > other platforms: There might be platforms that update Driver variables 
> > in BeforeConsole()
> > and expect these drivers are processed in the same boot (not the next 
> > boot). I don't have the real
> > examples. But today's flow allows this. With your change, Driver will 
> > not be processed in the first
> > boot. The change impacts these platforms.
> >
> > One backward compatible approach can be: processing Driver before 
> > BeforeConsole() and processing the new Driver (added by 
> > BeforeConsole()) after BeforeConsole().
> >
> > But another problem arises: If BeforeConsole() removes a Driver, 
> > platform's expectation is that deleted Driver doesn't run. But that 
> > driver already runs.
> >
> > So actually the question is: when BDS core can consume the Driver and 
> > process them?
> > Right now, it’s the time after BeforeConsole(). Just as right now 
> > BeforeConsole() updates ConOut
> > in your case, BeforeConsole() is the place where platform updates all UEFI 
> > defined boot related
> > variables. Processing Driver before BeforeConsole() requires platform 
> > updates Driver
> > in other places. It's a new requirement to the platform.
> >
> > With all what I explained in above, I cannot agree with the changes.
> >
> > Another approach is:
> > Platform could connect the GFX in AfterConsole() and update the ConOut. 
> > Then platform could manually call EfiBootManagerConnectConsoleVariable 
> > (ConOut) to re-connect ConOut.
> > It's a bit ugly I agree.
>
> Let me raise three other ideas (alternatives to each other, and to the 
> above), with varying levels of annoyance. :)
>

Thanks Laszlo

Ray, given your objection to my approach, could you please consider
the below and give feedback on which approach is suitable to address
the issue I am trying to fix?

>
> (1) Keep the following logic (i.e. the subject of this patch):
>
>   //
>   // Execute Driver Options
>   //
>   LoadOptions = EfiBootManagerGetLoadOptions (, 
> LoadOptionTypeDriver);
>   ProcessLoadOptions (LoadOptions, LoadOptionCount);
>   EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
>
> in *both* places, but gate each one with a bit in a new bitmask PCD.
>
> (Note: it's probably not the best for any platform to permit both branches, 
> as driver images would be loaded twice.)
>
>
> (2) EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL has recently been added to edk2. It 
> looks like a well-designed (extensible) protocol, for two reasons:
>
> - the protocol structure has a Revision field,
>
> - the only current member function, RefreshAllBootOptions(), is permitted to 
> return EFI_UNSUPPORTED -- and the single call site, in the 
> EfiBootManagerRefreshAllBootOption() function, handles that return value well 
> (it does nothing).
>
> The idea would be to bump the Revision field, and add a new member function. 
> Then call this member function (if it exists) in the spot where the current 
> patch proposes to move the Driver dispatch logic to.
>
> This is almost like a new PlatformBootManagerLib interface, except it does 
> not require existent lib instances to be updated.
>
> And, on the EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL implementation side, 
> RefreshAllBootOptions() would return EFI_UNSUPPORTED. (Because that is 
> irrelevant to Ard's use case.)
>
> Perhaps add yet another member function that can disable the Driver 
> option processing in the current location.
>
>
> (3) Extend the UEFI specification, section "3.1.3 Load Options".
>
> The LOAD_OPTION_CATEGORY bit-field is almost what we want, except it's 
> specified to be ignored for Driver options. So,
>
> - either invent the standardese to let us use LOAD_OPTION_CATEGORY for 
> Driver options too, without breaking existing platforms, *or*
> - introduce a new (not too wide) distinct bitfield called 
> LOAD_OPTION_DRIVER_CATEGORY.
>
> Whichever category bitfield proves acceptable, introduce a new "driver 
> category bit" in that bitfield, called LOAD_OPTION_DRIVER_CATEGORY_CONSOLE.
>
> Specify that, if any Driver option has the 
> LOAD_OPTION_DRIVER_CATEGORY_CONSOLE attribute set, then Driver options 
> will be processed in two passes. In both passes, DriverOrder is to be 
> honored, but the first pass will only consider options with the attribute 
> set, and the second pass will only consider options with the attribute clear.
>
> Implement this in BdsEntry / 

Re: [edk2-devel] [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for TPM2 measured boot

[Laszlo Ersek]

On 01/13/20 02:55, Gary Lin wrote:
> On Fri, Jan 10, 2020 at 12:32:02AM +, Yao, Jiewen wrote:
>> Hi Marc-André 
>> Would you please share some information on how to use vTPM with QEMU?
>>
>> I saw https://github.com/stefanberger/qemu-tpm
>>
>> But I am not sure if that has been integrated to official QEMU release?
>>
> Actually the TPM document can be found in the qemu package:
> https://github.com/qemu/qemu/blob/master/docs/specs/tpm.txt

Ugh, I've completely forgotten that this file has a part dedicated to
starting up the swtpm program!

The text file itself is referenced near the top of
"OvmfPkg/Include/IndustryStandard/QemuTpm.h". Now that I looked again at
the text file, I only expected to see the device interface, and was
surprised by the "swtpm" instructions :)

> I also maintained a wiki page for openSUSE:
> https://en.opensuse.org/Software_TPM_Emulator_For_QEMU

Very nice.

Thanks!
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53191): https://edk2.groups.io/g/devel/message/53191
Mute This Topic: https://groups.io/mt/69499023/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 1/1] BaseTools: Rationalise makefile generation

[PierreGondois]

From: Pierre Gondois 

The GenMake.py script tests the platform environment
to determine the type of makefile that needs to be
generated. If a Windows build host is detected, the
makefile generated is of Nmake type. Otherwise a
GNUmake type is generated.

Furthermore, the ___MAKE_PATH
option in tools_def.template defines the make tool
to use.
E.g.: for VS2017 this is configured to use Nmake, cf.
*_VS2017_*_MAKE_PATH = DEF(VS2017_BIN_HOST)\nmake.exe
while for GCC5 it is setup to use GNU make.
*_GCC5_*_MAKE_PATH = DEF(GCC_HOST_PREFIX)make

This prevents using the GCC compiler toolchain on a
Windows build host.

To address this issue this patch introduces 2 factors
to determine the generated makefile output.
  1. Platform -> to determine shell commands used
 in makefile.
  2. MakeTool -> to determine the type of makefile
 that needs to be generated.

Signed-off-by: Pierre Gondois 
Signed-off-by: Sami Mujawar 
---

The changes can be seen at 
https://github.com/PierreARM/edk2/tree/720_BaseTools_Rationalise_makefile_generation_v1

Notes:
v1:
  - Rationalise makefile generation [Pierre]

 BaseTools/Source/Python/AutoGen/GenMake.py | 122 ++--
 BaseTools/Source/Python/AutoGen/IncludesAutoGen.py |  34 +++---
 BaseTools/Source/Python/build/build.py |   7 +-
 3 files changed, 88 insertions(+), 75 deletions(-)

diff --git a/BaseTools/Source/Python/AutoGen/GenMake.py 
b/BaseTools/Source/Python/AutoGen/GenMake.py
index 
fe94f9a4c232bb599a59563444c3985700c78ec6..640b562257f74a45b38b8c857dae4750f7c286e0
 100755
--- a/BaseTools/Source/Python/AutoGen/GenMake.py
+++ b/BaseTools/Source/Python/AutoGen/GenMake.py
@@ -2,6 +2,7 @@
 # Create makefile for MS nmake and GNU make
 #
 # Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.
+# Copyright (c) 2020, ARM Limited. All rights reserved.
 # SPDX-License-Identifier: BSD-2-Clause-Patent
 #
 
@@ -52,13 +53,6 @@ gIncludeMacroConversion = {
   "EFI_PPI_DEPENDENCY"  :   gPpiDefinition,
 }
 
-## default makefile type
-gMakeType = ""
-if sys.platform == "win32":
-gMakeType = "nmake"
-else:
-gMakeType = "gmake"
-
 
 ## BuildFile class
 #
@@ -77,6 +71,13 @@ class BuildFile(object):
 "gmake" :   "GNUmakefile"
 }
 
+# Get Makefile name.
+def getMakefileName(self):
+if not self._FileType:
+return _DEFAULT_FILE_NAME_
+else:
+return self._FILE_NAME_[self._FileType]
+
 ## Fixed header string for makefile
 _MAKEFILE_HEADER = '''#
 # DO NOT EDIT
@@ -106,7 +107,7 @@ class BuildFile(object):
 #   $(RD) remove dir command
 #
 _SHELL_CMD_ = {
-"nmake" : {
+"win32" : {
 "CP":   "copy /y",
 "MV":   "move /y",
 "RM":   "del /f /q",
@@ -114,7 +115,7 @@ class BuildFile(object):
 "RD":   "rmdir /s /q",
 },
 
-"gmake" : {
+"posix" : {
 "CP":   "cp -f",
 "MV":   "mv -f",
 "RM":   "rm -f",
@@ -125,35 +126,35 @@ class BuildFile(object):
 
 ## directory separator
 _SEP_ = {
-"nmake" :   "\\",
-"gmake" :   "/"
+"win32" :   "\\",
+"posix" :   "/"
 }
 
 ## directory creation template
 _MD_TEMPLATE_ = {
-"nmake" :   'if not exist %(dir)s $(MD) %(dir)s',
-"gmake" :   "$(MD) %(dir)s"
+"win32" :   'if not exist %(dir)s $(MD) %(dir)s',
+"posix" :   "$(MD) %(dir)s"
 }
 
 ## directory removal template
 _RD_TEMPLATE_ = {
-"nmake" :   'if exist %(dir)s $(RD) %(dir)s',
-"gmake" :   "$(RD) %(dir)s"
+"win32" :   'if exist %(dir)s $(RD) %(dir)s',
+"posix" :   "$(RD) %(dir)s"
 }
 ## cp if exist
 _CP_TEMPLATE_ = {
-"nmake" :   'if exist %(Src)s $(CP) %(Src)s %(Dst)s',
-"gmake" :   "test -f %(Src)s && $(CP) %(Src)s %(Dst)s"
+"win32" :   'if exist %(Src)s $(CP) %(Src)s %(Dst)s',
+"posix" :   "test -f %(Src)s && $(CP) %(Src)s %(Dst)s"
 }
 
 _CD_TEMPLATE_ = {
-"nmake" :   'if exist %(dir)s cd %(dir)s',
-"gmake" :   "test -e %(dir)s && cd %(dir)s"
+"win32" :   'if exist %(dir)s cd %(dir)s',
+"posix" :   "test -e %(dir)s && cd %(dir)s"
 }
 
 _MAKE_TEMPLATE_ = {
-"nmake" :   'if exist %(file)s "$(MAKE)" $(MAKE_FLAGS) -f %(file)s',
-"gmake" :   'test -e %(file)s && "$(MAKE)" $(MAKE_FLAGS) -f %(file)s'
+"win32" :   'if exist %(file)s "$(MAKE)" $(MAKE_FLAGS) -f %(file)s',
+"posix" :   'test -e %(file)s && "$(MAKE)" $(MAKE_FLAGS) -f %(file)s'
 }
 
 _INCLUDE_CMD_ = {
@@ -169,22 +170,30 @@ class BuildFile(object):
 #
 def __init__(self, AutoGenObject):
 self._AutoGenObject = AutoGenObject
-self._FileType = gMakeType
 
-## Create build file
+MakePath = AutoGenObject.BuildOption.get('MAKE', {}).get('PATH')
+  

Re: [edk2-devel] [PATCH 1/3] MdeModulePkg/SdMmcPciHcDxe: Refactor command error detection

[Albecki, Mateusz]

Hi,

I will fix the 2 issues in separate patch probably before doing the refactor to 
avoid reverting it if the refactor introduces some unexpected issues.

Please also see inline.

Thanks,
Mateusz

> -Original Message-
> From: Wu, Hao A 
> Sent: Friday, January 10, 2020 6:38 AM
> To: Albecki, Mateusz ; devel@edk2.groups.io
> Cc: Marcin Wojtas ; Gao, Zhichao
> ; Gao, Liming 
> Subject: RE: [PATCH 1/3] MdeModulePkg/SdMmcPciHcDxe: Refactor
> command error detection
> 
> Hello Mateusz,
> 
> Some inline comments below:
> 
> 
> > -Original Message-
> > From: Albecki, Mateusz
> > Sent: Tuesday, January 07, 2020 7:06 PM
> > To: devel@edk2.groups.io
> > Cc: Albecki, Mateusz; Wu, Hao A; Marcin Wojtas; Gao, Zhichao; Gao,
> > Liming
> > Subject: [PATCH 1/3] MdeModulePkg/SdMmcPciHcDxe: Refactor
> command
> > error detection
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1140
> >
> > Error detection function will now check if the command failure has
> > been caused by one of the errors that can appear randomly on link(CRC
> > error + end bit error). If such an error has been a cause of failure
> > function will return EFI_CRC_ERROR instead of EFI_DEVICE_ERROR to
> > indicate to the higher level that command has a chance of succeeding
> > if resent. In addition this patch also fixes 2 small bugs. First one
> > is DAT lane being reset on current limit error. Second one is data
> > timeout error not being cleared after transfer has been completed.
> 
> 
> For the 2 small issues, I would suggest to split them into separate patches,
> which would make this patch into 3 patches. You can either do the
> refactoring or fixing the 2 bugs first.
> 
> 
> >
> > Cc: Hao A Wu 
> > Cc: Marcin Wojtas 
> > Cc: Zhichao Gao 
> > Cc: Liming Gao 
> >
> > Signed-off-by: Mateusz Albecki 
> > ---
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 234
> > +++
> >  1 file changed, 158 insertions(+), 76 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > index e7f2fac69b..8b5e54f321 100644
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > @@ -7,7 +7,7 @@
> >It would expose EFI_SD_MMC_PASS_THRU_PROTOCOL for upper layer
> use.
> >
> >Copyright (c) 2018-2019, NVIDIA CORPORATION. All rights reserved.
> > -  Copyright (c) 2015 - 2019, Intel Corporation. All rights
> > reserved.
> > +  Copyright (c) 2015 - 2020, Intel Corporation. All rights
> > + reserved.
> >SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >  **/
> > @@ -2137,6 +2137,154 @@ SdMmcExecTrb (
> >return Status;
> >  }
> >
> > +/**
> > +  Performs SW reset based on passed error status mask.
> > +
> > +  @param[in]  Private   Pointer to driver private data.
> > +  @param[in]  Slot  Index of the slot to reset.
> > +  @param[in]  ErrIntStatus  Error interrupt status mask.
> > +
> > +  @retval EFI_SUCCESS  Software reset performed successfully.
> > +  @retval OtherSoftware reset failed.
> > +**/
> > +EFI_STATUS
> > +SdMmcSoftwareReset (
> > +  IN SD_MMC_HC_PRIVATE_DATA  *Private,
> > +  IN UINT8   Slot,
> > +  IN UINT16  ErrIntStatus
> > +  )
> > +{
> > +  UINT8   SwReset;
> > +  EFI_STATUS  Status;
> > +
> > +  SwReset = 0;
> > +  if ((ErrIntStatus & 0x0F) != 0) {
> > +SwReset |= BIT1;
> > +  }
> > +  if ((ErrIntStatus & 0x70) != 0) {
> 
> 
> Thanks for this catch.
> Could you help to separate this fix to another patch?
> 
> 
> > +SwReset |= BIT2;
> > +  }
> > +
> > +  Status  = SdMmcHcRwMmio (
> > +  Private->PciIo,
> > +  Slot,
> > +  SD_MMC_HC_SW_RST,
> > +  FALSE,
> > +  sizeof (SwReset),
> > +  
> > +  );
> > +  if (EFI_ERROR (Status)) {
> > +return Status;
> > +  }
> > +
> > +  Status = SdMmcHcWaitMmioSet (
> > + Private->PciIo,
> > + Slot,
> > + SD_MMC_HC_SW_RST,
> > + sizeof (SwReset),
> > + 0xFF,
> > + 0,
> > + SD_MMC_HC_GENERIC_TIMEOUT
> > + );
> > +  if (EFI_ERROR (Status)) {
> > +return Status;
> > +  }
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > +  Checks the error status in error status register
> > +  and issues appropriate software reset as described in
> > +  SD specification section 3.10.
> > +
> > +  @param[in] Private  Pointer to driver private data.
> > +  @param[in] Trb  Pointer to currently executing TRB.
> > +
> > +  @retval EFI_CRC_ERROR  CRC error happened during CMD execution.
> > +  @retval EFI_SUCCESSNo error reported.
> > +  @retval Others Some other error happened.
> > +
> > +**/
> > +EFI_STATUS
> > +SdMmcCheckAndRecoverErrors (
> > +  IN SD_MMC_HC_PRIVATE_DATA  *Private,
> > +  IN UINT8   Slot
> > +  )
> > +{
> > +  UINT16  

Re: [edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles without device paths

[Ni, Ray]

We shouldn't assume that a DriverBindingStart() can only start on a handle with 
device path installed. DevicePath protocol is just a special protocol.
It's possible that a bus driver starts on a host controller handle and creates 
multiple children, each with only a Specific_IO protocol installed.
Certain device driver can start on the children handle and open the Specific_IO 
protocol BY_DRIVER.
I am not sure if certain today's network drivers may work like this. It's 
allowed per UEFI spec.

Thanks,
Ray

> -Original Message-
> From: vit9696 
> Sent: Monday, January 13, 2020 5:20 PM
> To: Gao, Zhichao 
> Cc: devel@edk2.groups.io; Ni, Ray 
> Subject: Re: [edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles 
> without device paths
> 
> Hi,
> 
> ‘Freeze’ means it is hung up forever, and we believe it is caused by an 
> invalid memory access in the firmware code.
> 
> Using loading command with ‘-nc’ (no connect) will work fine, but that is 
> kind of expected, as the issue is caused exactly by
> the connect logic.
> 
> We do not believe it is possible for device controller to exist without a 
> device path protocol in the wild. Would you please
> provide an example? If there is something we do not know about, I can imagine 
> introducing a PCD for this logic.
> 
> Best wishes,
> Vitaly
> 
> > 13 янв. 2020 г., в 11:11, Gao, Zhichao  написал(а):
> >
> >
> > Hi,
> >
> > What does 'freeze' mean? System blocked for a while or it hung up forever?
> >
> > The change would affect the operation of the 'load' command. If some 
> > drivers need to connect to the device controller
> but it doesn't have a device path protocol, the behavior of 'load' may be 
> incorrect.
> > Is the option '-nc' working for your request?
> >
> > Thanks,
> > Zhichao
> >
> >> -Original Message-
> >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> >> Vitaly Cheptsov via Groups.Io
> >> Sent: Monday, January 13, 2020 5:39 AM
> >> To: devel@edk2.groups.io
> >> Subject: [edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles without
> >> device paths
> >>
> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2460
> >>
> >> Doing this reduces the amount of needless work during device connection
> >> and resolves issues with firmwares that freeze when connecting handles
> >> without device paths.
> >>
> >> Signed-off-by: Vitaly Cheptsov 
> >> ---
> >> ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
> >> b/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
> >> index b6e7c952fa..083aac0dba 100644
> >> --- a/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
> >> +++ b/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
> >> @@ -32,8 +32,8 @@ ConnectAllEfi (
> >>   UINTN   Index;
> >>
> >>   Status = gBS->LocateHandleBuffer (
> >> -  AllHandles,
> >> -  NULL,
> >> +  ByProtocol,
> >> +  ,
> >>   NULL,
> >>   ,
> >>   
> >> --
> >> 2.21.0 (Apple Git-122.2)
> >>
> >>
> >> -=-=-=-=-=-=
> >> Groups.io Links: You receive all messages sent to this group.
> >>
> >> View/Reply Online (#53170): https://edk2.groups.io/g/devel/message/53170
> >> Mute This Topic: https://groups.io/mt/69653841/1768756
> >> Group Owner: devel+ow...@edk2.groups.io
> >> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> >> [zhichao@intel.com]
> >> -=-=-=-=-=-=
> >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53188): https://edk2.groups.io/g/devel/message/53188
Mute This Topic: https://groups.io/mt/69653841/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles without device paths

[Vitaly Cheptsov via Groups.Io]

Hi,

‘Freeze’ means it is hung up forever, and we believe it is caused by an invalid 
memory access in the firmware code.

Using loading command with ‘-nc’ (no connect) will work fine, but that is kind 
of expected, as the issue is caused exactly by the connect logic.

We do not believe it is possible for device controller to exist without a 
device path protocol in the wild. Would you please provide an example? If there 
is something we do not know about, I can imagine introducing a PCD for this 
logic.

Best wishes,
Vitaly

> 13 янв. 2020 г., в 11:11, Gao, Zhichao  написал(а):
> 
> 
> Hi,
> 
> What does 'freeze' mean? System blocked for a while or it hung up forever?
> 
> The change would affect the operation of the 'load' command. If some drivers 
> need to connect to the device controller but it doesn't have a device path 
> protocol, the behavior of 'load' may be incorrect.
> Is the option '-nc' working for your request?
> 
> Thanks,
> Zhichao
> 
>> -Original Message-
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>> Vitaly Cheptsov via Groups.Io
>> Sent: Monday, January 13, 2020 5:39 AM
>> To: devel@edk2.groups.io
>> Subject: [edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles without
>> device paths
>> 
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2460
>> 
>> Doing this reduces the amount of needless work during device connection
>> and resolves issues with firmwares that freeze when connecting handles
>> without device paths.
>> 
>> Signed-off-by: Vitaly Cheptsov 
>> ---
>> ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
>> b/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
>> index b6e7c952fa..083aac0dba 100644
>> --- a/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
>> +++ b/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
>> @@ -32,8 +32,8 @@ ConnectAllEfi (
>>   UINTN   Index;
>> 
>>   Status = gBS->LocateHandleBuffer (
>> -  AllHandles,
>> -  NULL,
>> +  ByProtocol,
>> +  ,
>>   NULL,
>>   ,
>>   
>> --
>> 2.21.0 (Apple Git-122.2)
>> 
>> 
>> -=-=-=-=-=-=
>> Groups.io Links: You receive all messages sent to this group.
>> 
>> View/Reply Online (#53170): https://edk2.groups.io/g/devel/message/53170
>> Mute This Topic: https://groups.io/mt/69653841/1768756
>> Group Owner: devel+ow...@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub
>> [zhichao@intel.com]
>> -=-=-=-=-=-=
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53187): https://edk2.groups.io/g/devel/message/53187
Mute This Topic: https://groups.io/mt/69653841/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

Re: [edk2-devel] [PATCH] MdePkg: PE loader should zero out dest buffer on allocation

[Marvin Häuser]

Good day,

Please see my comment in the related BZ: 
https://bugzilla.tianocore.org/show_bug.cgi?id=1999#c5


Best regards,
Marvin

Am 13.01.2020 um 09:18 schrieb Zhiguang Liu:

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1999

When PE loader loads image to memory, the first section of image may
not locate right next to the image header, which causes some memory
space remaining uninitialized. This is a security issue.
This patch compares the ending address of image header and the beginning
address of the first section. If there is a gap, zero out this gap.

Cc: Jian J Wang 
Cc: Hao A Wu 
Signed-off-by: Zhiguang Liu 
---
  MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 8 
  1 file changed, 8 insertions(+)

diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c 
b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
index 07bb62f860..2cdfb4a082 100644
--- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
+++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
@@ -1306,6 +1306,14 @@ PeCoffLoaderLoadImage (
// Load each section of the image
//
Section = FirstSection;
+  //
+  // Zero out the memory space between image header and the first section
+  //
+  End  = (CHAR8 *)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders);
+  Base = PeCoffLoaderImageAddress (ImageContext, Section->VirtualAddress, 
TeStrippedOffset);
+  if (End < Base) {
+ZeroMem (End, Base - End);
+  }
for (Index = 0; Index < NumberOfSections; Index++) {
  //
  // Read the section



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53186): https://edk2.groups.io/g/devel/message/53186
Mute This Topic: https://groups.io/mt/69664451/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH] MdePkg: PE loader should zero out dest buffer on allocation

[Zhiguang Liu]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1999

When PE loader loads image to memory, the first section of image may
not locate right next to the image header, which causes some memory
space remaining uninitialized. This is a security issue.
This patch compares the ending address of image header and the beginning
address of the first section. If there is a gap, zero out this gap.

Cc: Jian J Wang 
Cc: Hao A Wu 
Signed-off-by: Zhiguang Liu 
---
 MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c 
b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
index 07bb62f860..2cdfb4a082 100644
--- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
+++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
@@ -1306,6 +1306,14 @@ PeCoffLoaderLoadImage (
   // Load each section of the image
   //
   Section = FirstSection;
+  //
+  // Zero out the memory space between image header and the first section
+  //
+  End  = (CHAR8 *)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders);
+  Base = PeCoffLoaderImageAddress (ImageContext, Section->VirtualAddress, 
TeStrippedOffset);
+  if (End < Base) {
+ZeroMem (End, Base - End);
+  }
   for (Index = 0; Index < NumberOfSections; Index++) {
 //
 // Read the section
-- 
2.16.2.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53185): https://edk2.groups.io/g/devel/message/53185
Mute This Topic: https://groups.io/mt/69664451/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles without device paths

[Gao, Zhichao]

Hi,

What does 'freeze' mean? System blocked for a while or it hung up forever?

The change would affect the operation of the 'load' command. If some drivers 
need to connect to the device controller but it doesn't have a device path 
protocol, the behavior of 'load' may be incorrect.
Is the option '-nc' working for your request?

Thanks,
Zhichao

> -Original Message-
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Vitaly Cheptsov via Groups.Io
> Sent: Monday, January 13, 2020 5:39 AM
> To: devel@edk2.groups.io
> Subject: [edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles without
> device paths
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2460
> 
> Doing this reduces the amount of needless work during device connection
> and resolves issues with firmwares that freeze when connecting handles
> without device paths.
> 
> Signed-off-by: Vitaly Cheptsov 
> ---
>  ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
> b/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
> index b6e7c952fa..083aac0dba 100644
> --- a/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
> +++ b/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
> @@ -32,8 +32,8 @@ ConnectAllEfi (
>UINTN   Index;
> 
>Status = gBS->LocateHandleBuffer (
> -  AllHandles,
> -  NULL,
> +  ByProtocol,
> +  ,
>NULL,
>,
>
> --
> 2.21.0 (Apple Git-122.2)
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> 
> View/Reply Online (#53170): https://edk2.groups.io/g/devel/message/53170
> Mute This Topic: https://groups.io/mt/69653841/1768756
> Group Owner: devel+ow...@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> [zhichao@intel.com]
> -=-=-=-=-=-=


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53184): https://edk2.groups.io/g/devel/message/53184
Mute This Topic: https://groups.io/mt/69653841/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-