Re: [edk2-devel] [PATCH edk2 v2 3/3] StandaloneMmPkg: Fix check buffer address failed issue from TF-A

["Ming Huang" <huangming@linux.alibaba.com>]

Hi Marvin,

Apologize for missing your email. My thunderbird is odd that your email did't show in the thread.
Reply your comment in that email.

Thanks.

- Ming

在 12/22/21 9:01 PM, Marvin Häuser 写道:
> Hey Ming,
> 
> Please check https://edk2.groups.io/g/devel/message/84973?p=%2C%2C%2C20%2C0%2C0%2C0%3A%3Arecentpostdate%2Fsticky%2C%2CMarvin%2C20%2C2%2C0%2C86334815
> While a few comments were cosmetical, there is an invalid memory access outlined too, which persists in this patch.
> 
> For the future, this should also check buffer alignment before casting (lets hope my corresponding patch passes review within this decade...).
> 
> Best regards,
> Marvin
> 
> On 21.12.21 16:06, Ming Huang wrote:
>> There are two scene communicate with StandaloneMm(MM):
>> 1 edk2 -> TF-A -> MM, communicate MM use non-secure buffer which
>>    specify by EFI_SECURE_PARTITION_BOOT_INFO.SpNsCommBufBase;
>> 2 RAS scene: fiq -> TF-A -> MM, use secure buffer which
>>    specify by EFI_SECURE_PARTITION_BOOT_INFO.SpShareBufBase;
>>
>> For now, the second scene will failed because check buffer address.
>> This patch add CheckBufferAddr() to support check address for secure
>> buffer.
>>
>> Signed-off-by: Ming Huang <huangming@linux.alibaba.com>
>> ---
>>   StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c     | 59 +++++++++++++++-----
>>   StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21 +++++++
>>   StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h |  1 +
>>   3 files changed, 68 insertions(+), 13 deletions(-)
>>
>> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>> index 5dfaf9d751..ba8639a26a 100644
>> --- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>> +++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>> @@ -50,6 +50,7 @@ EFI_MM_COMMUNICATE_HEADER **PerCpuGuidedEventContext = NULL;
>>     // Descriptor with whereabouts of memory used for communication with the normal world
>>   EFI_MMRAM_DESCRIPTOR  mNsCommBuffer;
>> +EFI_MMRAM_DESCRIPTOR  mSCommBuffer;
>>     MP_INFORMATION_HOB_DATA *mMpInformationHobData;
>>   @@ -60,6 +61,47 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig = {
>>     STATIC EFI_MM_ENTRY_POINT     mMmEntryPoint = NULL;
>>   +STATIC
>> +EFI_STATUS
>> +CheckBufferAddr (
>> +  IN UINTN CommBufferAddr
>> +  )
>> +{
>> +  UINTN      CommBufferSize;
>> +  EFI_STATUS Status;
>> +  UINT64     NsCommBufferEnd;
>> +  UINT64     SCommBufferEnd;
>> +  UINT64     CommBufferEnd;
>> +
>> +  Status = EFI_SUCCESS;
>> +  NsCommBufferEnd = mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize;
>> +  SCommBufferEnd = mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize;
>> +
>> +  if ((CommBufferAddr >= mNsCommBuffer.PhysicalStart) &&
>> +      (CommBufferAddr < NsCommBufferEnd)) {
>> +    CommBufferEnd = NsCommBufferEnd;
>> +  } else if ((CommBufferAddr >= mSCommBuffer.PhysicalStart) &&
>> +             (CommBufferAddr <= SCommBufferEnd)) {
>> +    CommBufferEnd = SCommBufferEnd;
>> +  } else {
>> +    return EFI_ACCESS_DENIED;
>> +  }
>> +
>> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd) {
>> +    Status = EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  // Find out the size of the buffer passed
>> +  CommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) CommBufferAddr)->MessageLength +
>> +    sizeof (EFI_MM_COMMUNICATE_HEADER);
>> +  // perform bounds check.
>> +  if (CommBufferAddr + CommBufferSize >= CommBufferEnd) {
>> +    Status = EFI_ACCESS_DENIED;
>> +  }
>> +
>> +  return Status;
>> +}
>> +
>>   /**
>>     The PI Standalone MM entry point for the TF-A CPU driver.
>>   @@ -104,25 +146,16 @@ PiMmStandaloneArmTfCpuDriverEntry (
>>       return EFI_INVALID_PARAMETER;
>>     }
>>   -  if (NsCommBufferAddr < mNsCommBuffer.PhysicalStart) {
>> -    return EFI_ACCESS_DENIED;
>> -  }
>> -
>> -  if ((NsCommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>> -      (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>> -    return EFI_INVALID_PARAMETER;
>> +  Status = CheckBufferAddr (NsCommBufferAddr);
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((DEBUG_ERROR, "Check Buffer failed: %r\n", Status));
>> +    return Status;
>>     }
>>       // Find out the size of the buffer passed
>>     NsCommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) NsCommBufferAddr)->MessageLength +
>>       sizeof (EFI_MM_COMMUNICATE_HEADER);
>>   -  // perform bounds check.
>> -  if (NsCommBufferAddr + NsCommBufferSize >=
>> -      mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
>> -    return EFI_ACCESS_DENIED;
>> -  }
>> -
>>     GuidedEventContext = NULL;
>>     // Now that the secure world can see the normal world buffer, allocate
>>     // memory to copy the communication buffer to the secure world.
>> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
>> index fd9c59b4da..96dad20dd1 100644
>> --- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
>> +++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
>> @@ -107,6 +107,7 @@ StandaloneMmCpuInitialize (
>>     UINTN                            Index;
>>     UINTN                            ArraySize;
>>     VOID                            *HobStart;
>> +  EFI_MMRAM_HOB_DESCRIPTOR_BLOCK  *MmramRangesHob;
>>       ASSERT (SystemTable != NULL);
>>     mMmst = SystemTable;
>> @@ -186,6 +187,26 @@ StandaloneMmCpuInitialize (
>>     CopyMem (&mNsCommBuffer, NsCommBufMmramRange, sizeof(EFI_MMRAM_DESCRIPTOR));
>>     DEBUG ((DEBUG_INFO, "mNsCommBuffer: 0x%016lx - 0x%lx\n", mNsCommBuffer.CpuStart, mNsCommBuffer.PhysicalSize));
>>   +  Status = GetGuidedHobData (
>> +             HobStart,
>> +             &gEfiMmPeiMmramMemoryReserveGuid,
>> +             (VOID **) &MmramRangesHob
>> +             );
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((DEBUG_ERROR, "MmramRangesHob data extraction failed - 0x%x\n", Status));
>> +    return Status;
>> +  }
>> +
>> +  //
>> +  // As CreateHobListFromBootInfo(), the base and size of buffer shared with
>> +  // privileged Secure world software is in second one.
>> +  //
>> +  CopyMem (
>> +    &mSCommBuffer,
>> +    &MmramRangesHob->Descriptor[0] + 1,
>> +    sizeof(EFI_MMRAM_DESCRIPTOR)
>> +  );
>> +
>>     //
>>     // Extract the MP information from the Hoblist
>>     //
>> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
>> index 2c96439c15..2e03b20d85 100644
>> --- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
>> +++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
>> @@ -30,6 +30,7 @@ extern EFI_MM_CPU_PROTOCOL mMmCpuState;
>>   //
>>   extern EFI_MM_COMMUNICATE_HEADER    **PerCpuGuidedEventContext;
>>   extern EFI_MMRAM_DESCRIPTOR          mNsCommBuffer;
>> +extern EFI_MMRAM_DESCRIPTOR          mSCommBuffer;
>>   extern MP_INFORMATION_HOB_DATA       *mMpInformationHobData;
>>   extern EFI_MM_CONFIGURATION_PROTOCOL mMmConfig;
>>