Re: [PATCH v3 10/12] MdeModulePkg/SmmLockBox(PEI): Remove an ASSERT in RestoreLockBox()

[Laszlo Ersek <lersek@redhat.com>]

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 <jian.j.wang@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>
> Reviewed-by: Ray Ni <ray.ni@intel.com>
> ---
>  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 <lersek@redhat.com>

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