From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 DFBC22096FAC9 for ; Fri, 1 Feb 2019 01:39:43 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6AA4E40F19; Fri, 1 Feb 2019 09:39:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-49.rdu2.redhat.com [10.10.120.49]) by smtp.corp.redhat.com (Postfix) with ESMTP id 201185C22F; Fri, 1 Feb 2019 09:39:41 +0000 (UTC) To: Hao Wu , edk2-devel@lists.01.org Cc: Star Zeng References: <20190201054728.8612-1-hao.a.wu@intel.com> <20190201054728.8612-12-hao.a.wu@intel.com> From: Laszlo Ersek Message-ID: <1847a0dd-0012-ff73-78e1-7df46929e400@redhat.com> Date: Fri, 1 Feb 2019 10:39:40 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190201054728.8612-12-hao.a.wu@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Fri, 01 Feb 2019 09:39:43 +0000 (UTC) Subject: Re: [PATCH v3 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: Fri, 01 Feb 2019 09:39:44 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > 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 | 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.
> +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 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 (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. (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? 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. Thanks! Laszlo