public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-devel] [PATCH edk2 v1 3/3] StandaloneMmPkg: Fix check buffer address failed issue from TF-A
@ 2021-12-16  9:15 Marvin Häuser
  2021-12-23 10:46 ` Ming Huang
  0 siblings, 1 reply; 11+ messages in thread
From: Marvin Häuser @ 2021-12-16  9:15 UTC (permalink / raw)
  To: devel, huangming
  Cc: omkar.kulkarni, Sami Mujawar, ardb+tianocore, jiewen.yao,
	Supreeth Venkatesh, ming.huang-

Hey all,

> On 15. Dec 2021, at 16:02, Ming Huang <huangming@linux.alibaba.com> wrote:
> 
> 
> 
>> On 12/9/21 1:46 AM, Omkar Anand Kulkarni wrote:
>> 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 <huangming@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 = "
> 
> Modify it in v2.
> 
>> - 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 = "
> 
> Modify it in v2.
> 
>> - 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.
> 
> Your example is a good idea to solve this case. I may modify it like below in v2:
> 
> 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)) {

I find it odd the check here (lesser-equals) is inconsistent with the check above (lesser). It’d be caught below anyway, but I’d change this to lesser to keep the return codes consistent.

> CommBufferEnd = SCommBufferEnd;
> } else {
> return EFI_ACCESS_DENIED;
> }
> 
> if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd) {

Why is greater-equals used here? MessageLength == 0 is not filtered below, so this looks odd to be honest, as this is only the theoretical maximum buffer end.

How do you know this cannot wraparound? I actually don’t think we do. We do know it holds that CommBufferAddr <= CommBufferEnd though, so checking CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER) would give you that for free, if we assume the UINT64 variables above are actually bounded by UINTN, which seems reasonable - could ASSERT.

Alternatively, you could not store the maximum buffer end but the maximum buffer size, so the additions of the buffer start would just vanish. This might be more readable too I think.

> Status =  EFI_INVALID_PARAMETER;

Why is there no return here? This can proceed when the buffer cannot fit this header, and yet below the header is dereferenced.

> }
> 
> // Find out the size of the buffer passed
> CommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) CommBufferAddr)->MessageLength +
> sizeof (EFI_MM_COMMUNICATE_HEADER);

Same wraparound concern, same suggestion for solving it.

> // perform bounds check.
> if (CommBufferAddr + CommBufferSize >= CommBufferEnd) {
> Status =  EFI_ACCESS_DENIED;

It’s obviously not bad here, but for consistency’s sake, to mitigate bugs introduced by future changes, and readability, I’d return here and just return EFI_SUCCESS below, removing the code requirement of keeping Status consistent with the check results.

Finally, I really believe this kind of function should be abstracted in a way that it can be consumed by all places that accept any sort of communication buffer. Buffer validity checking is too critical than to duplicate it in every consumer.

Thanks!

Best regards,
Marvin

> }
> 
> return Status;
> }
> 
> - Ming
> 
>> 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],
>> - 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.
> 
> 
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread
* [PATCH edk2 v1 0/3] Fix several issues in StanaloneMmPkg
@ 2021-10-15  9:06 Ming Huang
  2021-10-15  9:06 ` [PATCH edk2 v1 3/3] StandaloneMmPkg: Fix check buffer address failed issue from TF-A Ming Huang
  0 siblings, 1 reply; 11+ messages in thread
From: Ming Huang @ 2021-10-15  9:06 UTC (permalink / raw)
  To: devel, sami.mujawar, ardb+tianocore, jiewen.yao,
	supreeth.venkatesh
  Cc: ming.huang-, Ming Huang

Fix issues in StandaloneMmPkg for supporting communicate from
TF-A with secure buffer.

Ming Huang (3):
  StandaloneMmPkg: Fix issue about SpPcpuSharedBufSize field
  StandaloneMmPkg: Replace DEBUG_INFO with DEBUG_ERROR
  StandaloneMmPkg: Fix check buffer address failed issue from TF-A

 .../Drivers/StandaloneMmCpu/EventHandle.c     | 76 +++++++++++++++----
 .../Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 33 ++++++--
 .../Drivers/StandaloneMmCpu/StandaloneMmCpu.h |  1 +
 .../Library/Arm/StandaloneMmCoreEntryPoint.h  |  2 +-
 .../Arm/CreateHobList.c                       |  2 +-
 .../Arm/StandaloneMmCoreEntryPoint.c          |  2 +-
 6 files changed, 91 insertions(+), 25 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-12-31 10:49 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-12-16  9:15 [edk2-devel] [PATCH edk2 v1 3/3] StandaloneMmPkg: Fix check buffer address failed issue from TF-A Marvin Häuser
2021-12-23 10:46 ` Ming Huang
2021-12-23 11:05   ` Marvin Häuser
2021-12-24  1:18     ` Ming Huang
2021-12-24 13:52       ` Marvin Häuser
2021-12-25  2:09         ` Ming Huang
2021-12-30 12:27           ` Marvin Häuser
2021-12-31 10:49             ` Ming Huang
  -- strict thread matches above, loose matches on Subject: below --
2021-10-15  9:06 [PATCH edk2 v1 0/3] Fix several issues in StanaloneMmPkg Ming Huang
2021-10-15  9:06 ` [PATCH edk2 v1 3/3] StandaloneMmPkg: Fix check buffer address failed issue from TF-A Ming Huang
2021-12-08 17:46   ` [edk2-devel] " Omkar Anand Kulkarni
2021-12-15 15:02     ` Ming Huang
2021-12-21 14:59     ` Ming Huang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox