From: "Wu, Hao A" <hao.a.wu@intel.com>
To: Laszlo Ersek <lersek@redhat.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Zeng, Star" <star.zeng@intel.com>
Subject: Re: [PATCH v3 11/12] MdeModulePkg/SmmLockBoxLib: Support LockBox enlarge in UpdateLockBox()
Date: Fri, 1 Feb 2019 11:03:56 +0000 [thread overview]
Message-ID: <B80AF82E9BFB8E4FBD8C89DA810C6A093C8946E8@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <1847a0dd-0012-ff73-78e1-7df46929e400@redhat.com>
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@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.wang@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Hao Wu <hao.a.wu@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 <lersek@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
next prev parent reply other threads:[~2019-02-01 11:04 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-01 5:47 [PATCH v3 00/12] Split the S3 PEI phase HW init codes from Opal driver Hao Wu
2019-02-01 5:47 ` [PATCH v3 01/12] MdeModulePkg: Add definitions for ATA AHCI host controller PPI Hao Wu
2019-02-01 5:47 ` [PATCH v3 02/12] MdeModulePkg: Add definitions for EDKII PEI ATA PassThru PPI Hao Wu
2019-02-01 5:47 ` [PATCH v3 03/12] MdeModulePkg: Add definitions for Storage Security Command PPI Hao Wu
2019-02-01 5:47 ` [PATCH v3 04/12] MdeModulePkg: Add GUID for LockBox to save storage dev to init in S3 Hao Wu
2019-02-01 5:47 ` [PATCH v3 05/12] MdeModulePkg/NvmExpressPei: Avoid updating the module-level variable Hao Wu
2019-02-01 5:47 ` [PATCH v3 06/12] MdeModulePkg/NvmExpressPei: Add logic to produce SSC PPI Hao Wu
2019-02-01 5:47 ` [PATCH v3 07/12] MdeModulePkg/NvmExpressPei: Consume S3StorageDeviceInitList LockBox Hao Wu
2019-02-01 5:47 ` [PATCH v3 08/12] MdeModulePkg/AhciPei: Add AHCI mode ATA device support in PEI Hao Wu
2019-02-01 5:47 ` [PATCH v3 09/12] MdeModulePkg/SmmLockBoxLib: Use 'DEBUG_' prefix instead of 'EFI_D_' Hao Wu
2019-02-01 9:04 ` Laszlo Ersek
2019-02-01 5:47 ` [PATCH v3 10/12] MdeModulePkg/SmmLockBox(PEI): Remove an ASSERT in RestoreLockBox() Hao Wu
2019-02-01 9:17 ` Laszlo Ersek
2019-02-01 10:54 ` Wu, Hao A
2019-02-01 5:47 ` [PATCH v3 11/12] MdeModulePkg/SmmLockBoxLib: Support LockBox enlarge in UpdateLockBox() Hao Wu
2019-02-01 7:52 ` Ni, Ray
2019-02-01 9:39 ` Laszlo Ersek
2019-02-01 11:03 ` Wu, Hao A [this message]
2019-02-01 5:47 ` [PATCH v3 12/12] SecurityPkg/OpalPassword: Remove HW init codes and consume SSC PPI Hao Wu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=B80AF82E9BFB8E4FBD8C89DA810C6A093C8946E8@SHSMSX104.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox