From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.100; helo=mga07.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 0C99E211C280D for ; Wed, 30 Jan 2019 21:58:26 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 30 Jan 2019 21:58:26 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,543,1539673200"; d="scan'208";a="112530412" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.127]) ([10.239.9.127]) by orsmga006.jf.intel.com with ESMTP; 30 Jan 2019 21:58:25 -0800 To: Hao Wu , edk2-devel@lists.01.org Cc: Jian J Wang , Ray Ni , Star Zeng References: <20190131024854.4880-1-hao.a.wu@intel.com> <20190131024854.4880-12-hao.a.wu@intel.com> From: "Ni, Ruiyu" Message-ID: Date: Thu, 31 Jan 2019 14:00:48 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20190131024854.4880-12-hao.a.wu@intel.com> Subject: Re: [PATCH v2 11/12] MdeModulePkg/SmmLockBoxLib: Support LockBox enlarge in UpdateLockBox() X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 31 Jan 2019 05:58:27 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Ray Ni > Cc: Star Zeng > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Hao Wu > --- > 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.
> +Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.
> > 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.
> +Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.
> > 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