> -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Friday, February 01, 2019 5:40 PM > To: Wu, Hao A; edk2-devel@lists.01.org > Cc: Zeng, Star > Subject: Re: [edk2] [PATCH v3 11/12] MdeModulePkg/SmmLockBoxLib: Support > LockBox enlarge in UpdateLockBox() > > Hi Hao, > > On 02/01/19 06:47, Hao Wu wrote: > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1409 > > > > This commit will add the support to enlarge a LockBox when using the > > LockBoxLib API UpdateLockBox(). > > > > Please note that the new support will ONLY work for LockBox with attribute > > LOCK_BOX_ATTRIBUTE_RESTORE_IN_S3_ONLY set. > > > > The functional uni-test for the commit is available at: > > https://github.com/hwu25/edk2/tree/lockbox_unitest > > > > Cc: Jian J Wang <jian.j.w...@intel.com> > > Cc: Ray Ni <ray...@intel.com> > > Cc: Star Zeng <star.z...@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Hao Wu <hao.a...@intel.com> > > --- > > MdeModulePkg/Include/Library/LockBoxLib.h | 7 +- > > MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.c | 7 +- > > MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c | 5 +- > > MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.c | 5 +- > > MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxSmmLib.c | 72 > ++++++++++++++++++-- > > 5 files changed, 86 insertions(+), 10 deletions(-) > > > > diff --git a/MdeModulePkg/Include/Library/LockBoxLib.h > b/MdeModulePkg/Include/Library/LockBoxLib.h > > index 5921731419..addce3bd4a 100644 > > --- a/MdeModulePkg/Include/Library/LockBoxLib.h > > +++ b/MdeModulePkg/Include/Library/LockBoxLib.h > > @@ -2,7 +2,7 @@ > > This library is only intended to be used by DXE modules that need save > > confidential information to LockBox and get it by PEI modules in S3 > > phase. > > > > -Copyright (c) 2010 - 2018, Intel Corporation. All rights reserved.<BR> > > +Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR> > > > > This program and the accompanying materials > > are licensed and made available under the terms and conditions > > @@ -85,7 +85,10 @@ SetLockBoxAttributes ( > > @retval RETURN_SUCCESS the information is saved successfully. > > @retval RETURN_INVALID_PARAMETER the Guid is NULL, or Buffer is NULL, > or Length is 0. > > @retval RETURN_NOT_FOUND the requested GUID not found. > > - @retval RETURN_BUFFER_TOO_SMALL the original buffer to too small to > hold new information. > > + @retval RETURN_BUFFER_TOO_SMALL for lockbox with attribute > LOCK_BOX_ATTRIBUTE_RESTORE_IN_PLACE, > > + the original buffer to too small to > > hold new > information. > > + @retval RETURN_OUT_OF_RESOURCES for lockbox with attribute > LOCK_BOX_ATTRIBUTE_RESTORE_IN_S3_ONLY, > > + no enough resource to save the > > information. > > @retval RETURN_ACCESS_DENIED it is too late to invoke this interface > > @retval RETURN_NOT_STARTED it is too early to invoke this > > interface > > @retval RETURN_UNSUPPORTED the service is not supported by > implementaion. > > diff --git a/MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.c > b/MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.c > > index c40dfea398..0adda1e2a9 100644 > > --- a/MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.c > > +++ b/MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.c > > @@ -1,6 +1,6 @@ > > /** @file > > > > -Copyright (c) 2010 - 2018, Intel Corporation. All rights reserved.<BR> > > +Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR> > > > > This program and the accompanying materials > > are licensed and made available under the terms and conditions > > @@ -76,7 +76,10 @@ SetLockBoxAttributes ( > > @retval RETURN_SUCCESS the information is saved successfully. > > @retval RETURN_INVALID_PARAMETER the Guid is NULL, or Buffer is NULL, > or Length is 0. > > @retval RETURN_NOT_FOUND the requested GUID not found. > > - @retval RETURN_BUFFER_TOO_SMALL the original buffer to too small to > hold new information. > > + @retval RETURN_BUFFER_TOO_SMALL for lockbox with attribute > LOCK_BOX_ATTRIBUTE_RESTORE_IN_PLACE, > > + the original buffer to too small to > > hold new > information. > > + @retval RETURN_OUT_OF_RESOURCES for lockbox with attribute > LOCK_BOX_ATTRIBUTE_RESTORE_IN_S3_ONLY, > > + no enough resource to save the > > information. > > @retval RETURN_ACCESS_DENIED it is too late to invoke this interface > > @retval RETURN_NOT_STARTED it is too early to invoke this > > interface > > @retval RETURN_UNSUPPORTED the service is not supported by > implementaion. > > diff --git a/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c > b/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c > > index 0428decbac..5ee563b71f 100644 > > --- a/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c > > +++ b/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c > > @@ -300,7 +300,10 @@ SetLockBoxAttributes ( > > @retval RETURN_SUCCESS the information is saved successfully. > > @retval RETURN_INVALID_PARAMETER the Guid is NULL, or Buffer is NULL, > or Length is 0. > > @retval RETURN_NOT_FOUND the requested GUID not found. > > - @retval RETURN_BUFFER_TOO_SMALL the original buffer to too small to > hold new information. > > + @retval RETURN_BUFFER_TOO_SMALL for lockbox with attribute > LOCK_BOX_ATTRIBUTE_RESTORE_IN_PLACE, > > + the original buffer to too small to > > hold new > information. > > + @retval RETURN_OUT_OF_RESOURCES for lockbox with attribute > LOCK_BOX_ATTRIBUTE_RESTORE_IN_S3_ONLY, > > + no enough resource to save the > > information. > > @retval RETURN_ACCESS_DENIED it is too late to invoke this interface > > @retval RETURN_NOT_STARTED it is too early to invoke this > > interface > > @retval RETURN_UNSUPPORTED the service is not supported by > implementaion. > > diff --git a/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.c > b/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.c > > index 8c3e65bc96..19fdd995c6 100644 > > --- a/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.c > > +++ b/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.c > > @@ -477,7 +477,10 @@ SetLockBoxAttributes ( > > @retval RETURN_SUCCESS the information is saved successfully. > > @retval RETURN_INVALID_PARAMETER the Guid is NULL, or Buffer is NULL, > or Length is 0. > > @retval RETURN_NOT_FOUND the requested GUID not found. > > - @retval RETURN_BUFFER_TOO_SMALL the original buffer to too small to > hold new information. > > + @retval RETURN_BUFFER_TOO_SMALL for lockbox with attribute > LOCK_BOX_ATTRIBUTE_RESTORE_IN_PLACE, > > + the original buffer to too small to > > hold new > information. > > + @retval RETURN_OUT_OF_RESOURCES for lockbox with attribute > LOCK_BOX_ATTRIBUTE_RESTORE_IN_S3_ONLY, > > + no enough resource to save the > > information. > > @retval RETURN_ACCESS_DENIED it is too late to invoke this interface > > @retval RETURN_NOT_STARTED it is too early to invoke this > > interface > > @retval RETURN_UNSUPPORTED the service is not supported by > implementaion. > > diff --git a/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxSmmLib.c > b/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxSmmLib.c > > index c912d187a4..d1cff97ba1 100644 > > --- a/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxSmmLib.c > > +++ b/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxSmmLib.c > > @@ -604,7 +604,10 @@ SetLockBoxAttributes ( > > @retval RETURN_SUCCESS the information is saved successfully. > > @retval RETURN_INVALID_PARAMETER the Guid is NULL, or Buffer is NULL, > or Length is 0. > > @retval RETURN_NOT_FOUND the requested GUID not found. > > - @retval RETURN_BUFFER_TOO_SMALL the original buffer to too small to > hold new information. > > + @retval RETURN_BUFFER_TOO_SMALL for lockbox with attribute > LOCK_BOX_ATTRIBUTE_RESTORE_IN_PLACE, > > + the original buffer to too small to > > hold new > information. > > + @retval RETURN_OUT_OF_RESOURCES for lockbox with attribute > LOCK_BOX_ATTRIBUTE_RESTORE_IN_S3_ONLY, > > + no enough resource to save the > > information. > > @retval RETURN_ACCESS_DENIED it is too late to invoke this interface > > @retval RETURN_NOT_STARTED it is too early to invoke this > > interface > > @retval RETURN_UNSUPPORTED the service is not supported by > implementaion. > > @@ -619,13 +622,16 @@ UpdateLockBox ( > > ) > > { > > SMM_LOCK_BOX_DATA *LockBox; > > + EFI_PHYSICAL_ADDRESS SmramBuffer; > > + EFI_STATUS Status; > > > > DEBUG ((DEBUG_INFO, "SmmLockBoxSmmLib UpdateLockBox - Enter\n")); > > > > // > > // Basic check > > // > > - if ((Guid == NULL) || (Buffer == NULL) || (Length == 0)) { > > + if ((Guid == NULL) || (Buffer == NULL) || (Length == 0) || > > + (Length > MAX_UINTN - Offset)) { > > DEBUG ((DEBUG_INFO, "SmmLockBoxSmmLib UpdateLockBox - Exit (%r)\n", > EFI_INVALID_PARAMETER)); > > return EFI_INVALID_PARAMETER; > > } > > @@ -643,8 +649,66 @@ UpdateLockBox ( > > // Update data > > // > > if (LockBox->Length < Offset + Length) { > > - DEBUG ((DEBUG_INFO, "SmmLockBoxSmmLib UpdateLockBox - Exit > (%r)\n", EFI_BUFFER_TOO_SMALL)); > > - return EFI_BUFFER_TOO_SMALL; > > + if ((LockBox->Attributes & > LOCK_BOX_ATTRIBUTE_RESTORE_IN_S3_ONLY) != 0) { > > + // > > + // If 'LOCK_BOX_ATTRIBUTE_RESTORE_IN_S3_ONLY' attribute is set, > enlarge the > > + // LockBox. > > + // > > + DEBUG (( > > + DEBUG_INFO, > > + "SmmLockBoxSmmLib UpdateLockBox - Origin LockBox too small, > enlarge.\n" > > + )); > > + > > + if (EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (LockBox->Length)) < Offset > + Length) { > > + // > > + // In SaveLockBox(), the SMRAM buffer allocated for LockBox is of > > page > > + // granularity. Here, if the required size is larger than the > > origin size > > + // of the pages, allocate new buffer from SMRAM to enlarge the > LockBox. > > + // > > + DEBUG (( > > + DEBUG_INFO, > > + "SmmLockBoxSmmLib UpdateLockBox - Allocate new buffer to > enlarge.\n" > > + )); > > + Status = gSmst->SmmAllocatePages ( > > + AllocateAnyPages, > > + EfiRuntimeServicesData, > > + EFI_SIZE_TO_PAGES (Offset + Length), > > + &SmramBuffer > > + ); > > + if (EFI_ERROR (Status)) { > > + DEBUG ((DEBUG_INFO, "SmmLockBoxSmmLib UpdateLockBox - Exit > (%r)\n", EFI_OUT_OF_RESOURCES)); > > + return EFI_OUT_OF_RESOURCES; > > + } > > + > > + // > > + // Copy origin data to the new SMRAM buffer and wipe the content in > the > > + // origin SMRAM buffer. > > + // > > + CopyMem ((VOID *)(UINTN)SmramBuffer, (VOID *)(UINTN)LockBox- > >SmramBuffer, (UINTN)LockBox->Length); > > + ZeroMem ((VOID *)(UINTN)LockBox->SmramBuffer, (UINTN)LockBox- > >Length); > > + gSmst->SmmFreePages (LockBox->SmramBuffer, EFI_SIZE_TO_PAGES > ((UINTN)LockBox->Length)); > > + > > + LockBox->SmramBuffer = SmramBuffer; > > + } > > + > > + // > > + // Handle potential uninitialized content in the LockBox. > > + // > > + if (Offset > LockBox->Length) { > > + ZeroMem ( > > + (VOID *)((UINTN)LockBox->SmramBuffer + (UINTN)LockBox->Length), > > + Offset - (UINTN)LockBox->Length > > + ); > > + } > > + LockBox->Length = Offset + Length; > > + } else { > > + // > > + // If 'LOCK_BOX_ATTRIBUTE_RESTORE_IN_S3_ONLY' attribute is NOT > set, return > > + // EFI_BUFFER_TOO_SMALL directly. > > + // > > + DEBUG ((DEBUG_INFO, "SmmLockBoxSmmLib UpdateLockBox - Exit > (%r)\n", EFI_BUFFER_TOO_SMALL)); > > + return EFI_BUFFER_TOO_SMALL; > > + } > > } > > ASSERT ((UINTN)LockBox->SmramBuffer <= (MAX_ADDRESS - Offset)); > > CopyMem ((VOID *)((UINTN)LockBox->SmramBuffer + Offset), Buffer, > Length); > > > > (1) The change is a no-op if LOCK_BOX_ATTRIBUTE_RESTORE_IN_S3_ONLY is > not set. > > As far as I can see, only the "SecurityPkg/Tcg/Opal/OpalPassword" driver > sets this attribute (both before, and after, patch v3 12/12 in this > series). So that's fine with me; OVMF does not include OpalPassword, > therefore this patch is a no-op even for the SMM_REQUIRE build of OVMF. > > Acked-by: Laszlo Ersek <ler...@redhat.com> > > > (2) In this patch, you modify the library class header, and then you > update some lockbox library instances as well -- just the documentation > -- whose behavior doesn't change. For example, the Null instance (where > no lockbox exists actually), and also the lib instances for PEIMs and > DXE+ drivers when the lockbox exists in SMRAM. > > That's great. However, the edk2 tree contains three more LockBoxLib > instances: > > OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf > OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf > Vlv2TbltDevicePkg/Library/I2CLibPei/I2CLibPei.inf > > Can you please do two more steps: > > (a) extend the UpdateLockBox() comments in > "OvmfPkg/Library/LockBoxLib/LockBoxLib.c", in a spearate patch. It can > be posted separately; no need to hold up this series just because of > that.
OK. If this series goes to v4, I will add another patch to address this. Otherwise, I will send a separate patch for this. > > (b) the LIBRARY_CLASS in > "Vlv2TbltDevicePkg/Library/I2CLibPei/I2CLibPei.inf" is in fact bogus. > The lib instance has nothing to do with the lockbox. Can you please post > a patch for fixing that define, or else file a BZ so that the > maintainers fix it? BZ filed first: https://bugzilla.tianocore.org/show_bug.cgi?id=1507 > > > My apologies that I'm only commenting on v3 -- I haven't noticed the > series earlier. In the future, please CC me on patches that are somehow > related to SMM. Sure. Sorry for missing you in the CC list. Best Regards, Hao Wu > > Thanks! > Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
