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 A98CB211BD5FC for ; Fri, 1 Feb 2019 01:17:50 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 29390AD892; Fri, 1 Feb 2019 09:17:50 +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 F4224608DA; Fri, 1 Feb 2019 09:17:48 +0000 (UTC) To: Hao Wu , edk2-devel@lists.01.org Cc: Star Zeng References: <20190201054728.8612-1-hao.a.wu@intel.com> <20190201054728.8612-11-hao.a.wu@intel.com> From: Laszlo Ersek Message-ID: <1d5d58ca-50d8-e365-7507-8666f7eebdde@redhat.com> Date: Fri, 1 Feb 2019 10:17:47 +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-11-hao.a.wu@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Fri, 01 Feb 2019 09:17:50 +0000 (UTC) Subject: Re: [PATCH v3 10/12] MdeModulePkg/SmmLockBox(PEI): Remove an ASSERT in RestoreLockBox() 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:17:50 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/01/19 06:47, Hao Wu wrote: > This commit is out of the scope for BZ-1409. It is a refinement for the > PEI library instance within SmmLockBoxLib. > > For the below ASSERT statement within function RestoreLockBox(): > Status = SmmCommunicationPpi->Communicate ( > SmmCommunicationPpi, > &CommBuffer[0], > &CommSize > ); > if (Status == EFI_NOT_STARTED) { > // > // Pei SMM communication not ready yet, so we access SMRAM directly > // > DEBUG ((DEBUG_INFO, "SmmLockBoxPeiLib Communicate - (%r)\n", Status)); > Status = InternalRestoreLockBoxFromSmram (Guid, Buffer, Length); > LockBoxParameterRestore->Header.ReturnStatus = (UINT64)Status; > if (Length != NULL) { > LockBoxParameterRestore->Length = (UINT64)*Length; > } > } > ASSERT_EFI_ERROR (Status); > > It is possible for previous codes to return an error status that is > possible for happen. One example is that, when the 'if' statement > 'if (Status == EFI_NOT_STARTED) {' is entered, function > InternalRestoreLockBoxFromSmram() is possible to return 'BUFFER_TOO_SMALL' > if the caller of RestoreLockBox() provides a buffer that is too small to > hold the content of LockBox. > > Thus, this commit will remove the ASSERT here. > > Please note that the current implementation of RestoreLockBox() is > handling the above-mentioned error case properly, so no additional error > handling codes are needed here. > > Cc: Jian J Wang > Cc: Star Zeng > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Hao Wu > Reviewed-by: Ray Ni > --- > MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.c b/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.c > index 9f73480070..8c3e65bc96 100644 > --- a/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.c > +++ b/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.c > @@ -623,7 +623,6 @@ RestoreLockBox ( > LockBoxParameterRestore->Length = (UINT64)*Length; > } > } > - ASSERT_EFI_ERROR (Status); > > if (Length != NULL) { > *Length = (UINTN)LockBoxParameterRestore->Length; > OVMF never reaches this code path because it doesn't include an EFI_PEI_SMM_COMMUNICATION_PPI instance. Therefore OVMF always goes through InternalRestoreLockBoxFromSmram(). See commit bd3afeb1d62c ("MdeModulePkg: SmmLockBoxPeiLib: work without EFI_PEI_SMM_COMMUNICATION_PPI", 2015-11-16). In that regard, I'm OK with this patch; this alone would suffice for me to give an Acked-by. However, having re-reviewed bd3afeb1d62c now, I see that this patch is actually technically correct. So I believe I can give an R-b too, despite OVMF not using the affected code path. Reviewed-by: Laszlo Ersek Other than that, you might want to review existing callers of this function, to ensure they don't rely on any such failure being caught internally to the function (via the ASSERT that's now being removed). Again, this would only be relevant for platforms that produce an EFI_PEI_SMM_COMMUNICATION_PPI instance. Thanks Laszlo