在 12/9/21 1:46 AM, Omkar Anand Kulkarni 写道:
> Hi Ming,
>
> Thanks for this patch. This patch helps to resolve Standalone MM issue while
> exercising RAS use case.
> Few comments mentioned inline.
>
> - Omkar
>
>
> On 10/15/21 2:39 PM, Ming Huang via groups.io 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 <huangm...@linux.alibaba.com>
>> ---
>> StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c | 70
>> ++++++++++++++++----
>> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21
>> ++++++
>> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h | 1 +
>> 3 files changed, 79 insertions(+), 13 deletions(-)
>>
>> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>> b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>> index 5dfaf9d751..63fab1bd78 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,58 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig =
>> {
>>
>> STATIC EFI_MM_ENTRY_POINT mMmEntryPoint = NULL;
>>
>> +STATIC
>> +EFI_STATUS
>> +CheckBufferAddr (
>> + IN UINTN CommBufferAddr
>> + )
>> +{
>> + UINTN CommBufferSize;
>> + EFI_STATUS Status;
>> +
>> + Status = EFI_SUCCESS;
>> + if (CommBufferAddr < mNsCommBuffer.PhysicalStart) {
>> + Status = EFI_ACCESS_DENIED;
>> + }
>> +
>> + if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>> + (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>> + Status = EFI_INVALID_PARAMETER;
>
> Single space after "Status = "
>
> - Omkar
>
>
>> + }
>> +
>> + // 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 >=
>> + mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
>> + Status = EFI_ACCESS_DENIED;
>
> Single space after "Status = "
>
> - Omkar
>
>> + }
>> +
>> + if (!EFI_ERROR (Status)) {
>
>
> In case of error this function call will not return from here. It will
> execute the code below comparing the MM Communicate buffer address with the
> Secure buffer address, which may cause wrong return type being returned. Can
> you check this, please?
>
> - Omkar
>
>
>> + return EFI_SUCCESS;
>> + }
>> +
>> + Status = EFI_SUCCESS;
>> + if (CommBufferAddr < mSCommBuffer.PhysicalStart) {
>> + Status = EFI_ACCESS_DENIED;
>> + }
>> +
>> + if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>> + (mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize)) {
>> + Status = EFI_INVALID_PARAMETER;
>> + }
>> +
>> + // perform bounds check.
>> + if (CommBufferAddr + CommBufferSize >=
>> + mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize) {
>> + Status = EFI_ACCESS_DENIED;
>> + }
>> +
>> + return Status;
>> +}
>> +
>
>
> CheckBufferAddr() function performs validity and overflow checks on the
> Communication buffers. These checks are same for both the non-secure
> MM communicate buffer and secure buffer shared between EL3 and S-EL0. Can
> this code be combined ( example below)? This will help mitigate the above
> mentioned return type issue as well.
>
> STATIC
> EFI_STATUS
> CheckBufferAddr (
> IN UINTN CommBufferAddr
> )
> {
> UINTN CommBufferSize;
> EFI_STATUS Status;
> EFI_MMRAM_DESCRIPTOR CommBuffer;
>
> if (CommBufferAddr < mNsCommBuffer.PhysicalStart ||
> CommBufferAddr > (mNsCommBuffer.PhysicalStart +
> mNsCommBuffer.PhysicalSize)) {
> CommBuffer = mSCommBuffer;
> } else {
> CommBuffer = mNsCommBuffer;
> }
>
> if (CommBufferAddr < CommBuffer.PhysicalStart) {
> Status = EFI_ACCESS_DENIED;
> }
>
> if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
> (CommBuffer.PhysicalStart + CommBuffer.PhysicalSize)) {
> 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 >=
> CommBuffer.PhysicalStart + CommBuffer.PhysicalSize) {
> Status = EFI_ACCESS_DENIED;
> }
>
> return Status;
> }
>
> - Omkar
>
>
>> /**
>> The PI Standalone MM entry point for the TF-A CPU driver.
>>
>> @@ -104,25 +157,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,
>
> Can this be changed to
> &MmramRangesHob->Descriptor[1],
I missed this comment at last email.
The struct define:
EFI_MMRAM_DESCRIPTOR Descriptor[1];
I guess some static code analysis tool may report out of array range, if modify
to
&MmramRangesHob->Descriptor[1].
- Ming
>
> - Omkar
>
>> + 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;
>>
>> --
>> 2.17.1
>>
>>
>>
>>
>>
>
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium. Thank you.
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#85130): https://edk2.groups.io/g/devel/message/85130
Mute This Topic: https://groups.io/mt/86334815/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
