Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/ReportStatusCodeRouter: Revert end pointer on out of resources

["Dandan Bi" <dandan.bi@intel.com>]

Reviewed-by: Dandan Bi <dandan.bi@intel.com>


Thanks,
Dandan
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Michael Kubacki
> Sent: Saturday, April 11, 2020 4:50 AM
> To: devel@edk2.groups.io
> Cc: Bi, Dandan <dandan.bi@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Wang, Jian J <jian.j.wang@intel.com>; Kun Qin <Kun.Qin@microsoft.com>;
> Gao, Liming <liming.gao@intel.com>
> Subject: [edk2-devel] [PATCH v1 1/1]
> MdeModulePkg/ReportStatusCodeRouter: Revert end pointer on out of
> resources
> 
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2665
> 
> ReportDispatcher() is called by a software module to report a status code.
> The interface is generic and can be called frequently throughout the boot
> under various conditions. A certain set of conditions can cause the currently
> implemented algorithm for resource exhaustion to fail. A sample
> scenario:
> 
> 1. ReportStatusCode() is called at a TPL higher than one of the registered
>    status code listeners making the call to the listener deferred until
>    TPL is lowered.
> 2. Additional calls to ReportStatusCode() occur, so the data buffer
>    continues to expand.
> 3. A call to ReportStatusCode() is made from within a memory allocation
>    call (e.g. CoreAllocatePoolPages ()) which is protected from re-
>    entrancy with mPoolMemoryLock. This will cause the ReallocatePool()
>    call in ReportDispatcher() to fail. Because the end pointer was already
>    moved to account for the data size, the end pointer is now moved
>    beyond the buffer and invalid.
> 
> This commit saves the original end pointer value into a local variable called
> "FailSafeEndPointer" which tracks a safe end pointer to revert to in the case
> the allocated buffer size (CallbackEntry->EndPointer -
> CallbackEntry->StatusCodeDataBuffer) is still not large enough for the
> data.
> 
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Kun Qin <Kun.Qin@microsoft.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> 
> MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/ReportSt
> atusCodeRouterRuntimeDxe.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git
> a/MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/Report
> StatusCodeRouterRuntimeDxe.c
> b/MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/Report
> StatusCodeRouterRuntimeDxe.c
> index 6ca7e180ebb3..d7dc0a75ac83 100644
> ---
> a/MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/Report
> StatusCodeRouterRuntimeDxe.c
> +++
> b/MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/Report
> Sta
> +++ tusCodeRouterRuntimeDxe.c
> @@ -3,6 +3,7 @@
>    and Status Code Runtime Protocol.
> 
>    Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) Microsoft Corporation.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -237,6 +238,7 @@ ReportDispatcher (
>    RSC_DATA_ENTRY                *RscData;
>    EFI_STATUS                    Status;
>    VOID                          *NewBuffer;
> +  EFI_PHYSICAL_ADDRESS          FailSafeEndPointer;
> 
>    //
>    // Use atom operation to avoid the reentant of report.
> @@ -267,6 +269,7 @@ ReportDispatcher (
>      // If callback is registered with TPL lower than TPL_HIGH_LEVEL, event
> must be signaled at boot time to possibly wait for
>      // allowed TPL to report status code. Related data should also be stored in
> data buffer.
>      //
> +    FailSafeEndPointer = CallbackEntry->EndPointer;
>      CallbackEntry->EndPointer  = ALIGN_VARIABLE (CallbackEntry-
> >EndPointer);
>      RscData = (RSC_DATA_ENTRY *) (UINTN) CallbackEntry->EndPointer;
>      CallbackEntry->EndPointer += sizeof (RSC_DATA_ENTRY); @@ -285,6
> +288,7 @@ ReportDispatcher (
>                        (VOID *) (UINTN) CallbackEntry->StatusCodeDataBuffer
>                        );
>          if (NewBuffer != NULL) {
> +          FailSafeEndPointer = (EFI_PHYSICAL_ADDRESS) (UINTN) NewBuffer
> + + (FailSafeEndPointer - CallbackEntry->StatusCodeDataBuffer);
>            CallbackEntry->EndPointer = (EFI_PHYSICAL_ADDRESS) (UINTN)
> NewBuffer + (CallbackEntry->EndPointer - CallbackEntry-
> >StatusCodeDataBuffer);
>            CallbackEntry->StatusCodeDataBuffer = (EFI_PHYSICAL_ADDRESS)
> (UINTN) NewBuffer;
>            CallbackEntry->BufferSize *= 2; @@ -296,6 +300,7 @@
> ReportDispatcher (
>      // If data buffer is used up, do not report for this time.
>      //
>      if (CallbackEntry->EndPointer > (CallbackEntry->StatusCodeDataBuffer +
> CallbackEntry->BufferSize)) {
> +      CallbackEntry->EndPointer = FailSafeEndPointer;
>        continue;
>      }
> 
> --
> 2.16.3.windows.1
> 
> 
>