Re: [PATCH] UefiCpuPkg/MpInitLib: fix issue in wakeup buffer initialization

[Laszlo Ersek <lersek@redhat.com>]

On 01/24/18 03:08, Jian J Wang wrote:
> To fix an issue in which enabling NX feature will mark the AP wakeup
> buffer as non-executable and fail the AP init, the buffer was split
> into two part: the lower part in memory within 1MB and the higher part
> within allocated executable memory (EfiBootServicesCode). But the
> address of higher part memory was stored in lower part memory, which
> is actually shared with legacy components and will be overwritten by
> LegacyBiosDxe driver if CSM is enabled.
> 
> This patch fixes this issue by storing the address of higher part
> memory in CpuMpData instead of ExchangeInfo.
> 
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 20 ++++++++++----------
>  UefiCpuPkg/Library/MpInitLib/MpLib.h |  1 +
>  2 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 6231968c74..42011d6231 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -823,19 +823,20 @@ FillExchangeInfoData (
>    // Copy all 32-bit code and 64-bit code into memory with type of
>    // EfiBootServicesCode to avoid page fault if NX memory protection is enabled.
>    //
> -  if (ExchangeInfo->ModeTransitionMemory != 0) {
> +  if (CpuMpData->WakeupBufferHigh != 0) {
>      Size = CpuMpData->AddressMap.RendezvousFunnelSize -
>             CpuMpData->AddressMap.ModeTransitionOffset;
>      CopyMem (
> -      (VOID *)(UINTN)ExchangeInfo->ModeTransitionMemory,
> +      (VOID *)CpuMpData->WakeupBufferHigh,
>        CpuMpData->AddressMap.RendezvousFunnelAddress +
>        CpuMpData->AddressMap.ModeTransitionOffset,
>        Size
>        );
>  
> -    ExchangeInfo->ModeHighMemory = ExchangeInfo->ModeTransitionMemory;
> -    ExchangeInfo->ModeHighMemory += (UINT32)ExchangeInfo->ModeOffset -
> -               (UINT32)CpuMpData->AddressMap.ModeTransitionOffset;
> +    ExchangeInfo->ModeTransitionMemory = (UINT32)CpuMpData->WakeupBufferHigh;
> +    ExchangeInfo->ModeHighMemory = (UINT32)CpuMpData->WakeupBufferHigh +
> +                                   (UINT32)ExchangeInfo->ModeOffset -
> +                                   (UINT32)CpuMpData->AddressMap.ModeTransitionOffset;
>      ExchangeInfo->ModeHighSegment = (UINT16)ExchangeInfo->CodeSegment;
>    } else {
>      ExchangeInfo->ModeTransitionMemory = (UINT32)
> @@ -916,11 +917,10 @@ AllocateResetVector (
>      CpuMpData->WakeupBuffer      = GetWakeupBuffer (ApResetVectorSize);
>      CpuMpData->MpCpuExchangeInfo = (MP_CPU_EXCHANGE_INFO *) (UINTN)
>                      (CpuMpData->WakeupBuffer + CpuMpData->AddressMap.RendezvousFunnelSize);
> -    CpuMpData->MpCpuExchangeInfo->ModeTransitionMemory = (UINT32)
> -                    GetModeTransitionBuffer (
> -                      CpuMpData->AddressMap.RendezvousFunnelSize -
> -                      CpuMpData->AddressMap.ModeTransitionOffset
> -                      );
> +    CpuMpData->WakeupBufferHigh  = GetModeTransitionBuffer (
> +                                    CpuMpData->AddressMap.RendezvousFunnelSize -
> +                                    CpuMpData->AddressMap.ModeTransitionOffset
> +                                    );
>    }
>    BackupAndPrepareWakeupBuffer (CpuMpData);
>  }
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 0232fe896a..e7f9a4de0a 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -208,6 +208,7 @@ struct _CPU_MP_DATA {
>    UINTN                          CpuApStackSize;
>    MP_ASSEMBLY_ADDRESS_MAP        AddressMap;
>    UINTN                          WakeupBuffer;
> +  UINTN                          WakeupBufferHigh;
>    UINTN                          BackupBuffer;
>    UINTN                          BackupBufferSize;
>  
> 

As far as I see, all the values stored by FillExchangeInfoData() remain
the same. The difference is that "ModeTransitionMemory" becomes an
*output* field for FillExchangeInfoData(), from being an input field. We
now explicitly store the value to it that we previously *assumed* it
would hold (preserve), from AllocateResetVector().

The input now comes through WakeupBufferHigh, from the
AllocateResetVector() function.

Thus, I think the patch could have been written a bit more simply: we
could have added the assignment

ExchangeInfo->ModeTransitionMemory = (UINT32)CpuMpData->WakeupBufferHigh

before the "if" in FillExchangeInfoData(), and then the rest of
FillExchangeInfoData() would have needed no changes. But perhaps that
would have been harder to understand.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo