From: "Ni, Ruiyu" <ruiyu.ni@Intel.com>
To: Hao Wu <hao.a.wu@intel.com>, edk2-devel@lists.01.org
Cc: Jian J Wang <jian.j.wang@intel.com>, Ray Ni <ray.ni@intel.com>,
Star Zeng <star.zeng@intel.com>
Subject: Re: [PATCH v2 11/12] MdeModulePkg/SmmLockBoxLib: Support LockBox enlarge in UpdateLockBox()
Date: Thu, 31 Jan 2019 14:00:48 +0800 [thread overview]
Message-ID: <a89c8902-0ba5-c19a-61ea-6df1bdaeeb72@Intel.com> (raw)
In-Reply-To: <20190131024854.4880-12-hao.a.wu@intel.com>
On 1/31/2019 10:48 AM, 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 | 57 ++++++++++++++++++--
> 5 files changed, 72 insertions(+), 9 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 9d7b4c3706..fa558e5c69 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..095cf8f8a7 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,6 +622,8 @@ UpdateLockBox (
> )
> {
> SMM_LOCK_BOX_DATA *LockBox;
> + EFI_PHYSICAL_ADDRESS SmramBuffer;
> + EFI_STATUS Status;
>
> DEBUG ((DEBUG_INFO, "SmmLockBoxSmmLib UpdateLockBox - Enter\n"));
>
> @@ -643,8 +648,54 @@ 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, re-allocate
> + // buffer from SMRAM to expand the LockBox.
> + //
> + DEBUG ((
> + DEBUG_INFO,
> + "SmmLockBoxSmmLib UpdateLockBox - Origin LockBox too small, expand.\n"
> + ));
> +
> + //
> + // Allocate new SMRAM buffer
> + //
> + Status = gSmst->SmmAllocatePages (
> + AllocateAnyPages,
> + EfiRuntimeServicesData,
> + EFI_SIZE_TO_PAGES (Offset + Length),
> + &SmramBuffer
> + );
> + ASSERT_EFI_ERROR (Status);
1. Remove the assertion?
2. SaveLockBox() allocates memory in page granularity. So it's possible
that the allocation here is unnecessary. For example:
SaveLockBox() requires 30 bytes. 4K bytes is actually allocated.
UpdateLockBox() requries 50 bytes in total. 4K bytes is required actually.
The original 4K is still enough for the new lock box data.
Maybe we could assume the capacity is always 4K aligned, so the
unnecessary allocation can be avoided in most of the cases.
3. Another thing I am not sure is do we need to check whether
Offset + Length overflows? The check doesn't exist in old code so
it's a off-topic comment.
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_INFO, "SmmLockBoxSmmLib UpdateLockBox - Exit (%r)\n", EFI_OUT_OF_RESOURCES));
> + return EFI_OUT_OF_RESOURCES;
> + }
> + ZeroMem ((VOID *)(UINTN)SmramBuffer, Offset + Length);
> +
> + //
> + // Copy origin data to the new SMRAM buffer and wipe the content in the
> + // origin SMRAM
> + //
> + 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));
> +
> + //
> + // Update information in SMM_LOCK_BOX_DATA structure
> + //
> + LockBox->Buffer = (EFI_PHYSICAL_ADDRESS)(UINTN)Buffer;
> + LockBox->Length = Offset + Length;
> + LockBox->SmramBuffer = SmramBuffer;
> + } 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);
>
--
Thanks,
Ray
next prev parent reply other threads:[~2019-01-31 5:58 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-31 2:48 [PATCH v2 00/12] Split the S3 PEI phase HW init codes from Opal driver Hao Wu
2019-01-31 2:48 ` [PATCH v2 01/12] MdeModulePkg: Add definitions for ATA AHCI host controller PPI Hao Wu
2019-01-31 3:25 ` Ni, Ray
2019-01-31 2:48 ` [PATCH v2 02/12] MdeModulePkg: Add definitions for EDKII PEI ATA PassThru PPI Hao Wu
2019-01-31 3:22 ` Ni, Ray
2019-01-31 5:28 ` Wu, Hao A
2019-01-31 2:48 ` [PATCH v2 03/12] MdeModulePkg: Add definitions for Storage Security Command PPI Hao Wu
2019-01-31 3:26 ` Ni, Ray
2019-01-31 2:48 ` [PATCH v2 04/12] MdeModulePkg: Add GUID for LockBox to save storage dev to init in S3 Hao Wu
2019-01-31 3:27 ` Ni, Ray
2019-01-31 5:30 ` Wu, Hao A
2019-01-31 2:48 ` [PATCH v2 05/12] MdeModulePkg/NvmExpressPei: Avoid updating the module-level variable Hao Wu
2019-01-31 3:28 ` Ni, Ray
2019-01-31 2:48 ` [PATCH v2 06/12] MdeModulePkg/NvmExpressPei: Add logic to produce SSC PPI Hao Wu
2019-01-31 3:35 ` Ni, Ray
2019-01-31 5:40 ` Wu, Hao A
2019-01-31 2:48 ` [PATCH v2 07/12] MdeModulePkg/NvmExpressPei: Consume S3StorageDeviceInitList LockBox Hao Wu
2019-01-31 3:45 ` Ni, Ray
2019-01-31 5:45 ` Wu, Hao A
2019-01-31 2:48 ` [PATCH v2 08/12] MdeModulePkg/AhciPei: Add AHCI mode ATA device support in PEI Hao Wu
2019-01-31 5:49 ` Ni, Ruiyu
2019-01-31 2:48 ` [PATCH v2 09/12] MdeModulePkg/SmmLockBoxLib: Use 'DEBUG_' prefix instead of 'EFI_D_' Hao Wu
2019-01-31 5:49 ` Ni, Ruiyu
2019-01-31 2:48 ` [PATCH v2 10/12] MdeModulePkg/SmmLockBox(PEI): Remove an ASSERT in RestoreLockBox() Hao Wu
2019-01-31 5:50 ` Ni, Ruiyu
2019-01-31 5:53 ` Wu, Hao A
2019-01-31 2:48 ` [PATCH v2 11/12] MdeModulePkg/SmmLockBoxLib: Support LockBox enlarge in UpdateLockBox() Hao Wu
2019-01-31 6:00 ` Ni, Ruiyu [this message]
2019-01-31 2:48 ` [PATCH v2 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=a89c8902-0ba5-c19a-61ea-6df1bdaeeb72@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