From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out30-45.freemail.mail.aliyun.com (out30-45.freemail.mail.aliyun.com [115.124.30.45]) by mx.groups.io with SMTP id smtpd.web10.31368.1640255929342611656 for ; Thu, 23 Dec 2021 02:38:50 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: linux.alibaba.com, ip: 115.124.30.45, mailfrom: huangming@linux.alibaba.com) X-Alimail-AntiSpam: AC=PASS;BC=-1|-1;BR=01201311R991e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=e01e04357;MF=huangming@linux.alibaba.com;NM=1;PH=DS;RN=7;SR=0;TI=SMTPD_---0V.XBnO9_1640255923; Received: from 30.18.74.6(mailfrom:huangming@linux.alibaba.com fp:SMTPD_---0V.XBnO9_1640255923) by smtp.aliyun-inc.com(127.0.0.1); Thu, 23 Dec 2021 18:38:44 +0800 Message-ID: Date: Thu, 23 Dec 2021 18:38:43 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.4.1 Subject: Re: [edk2-devel] [PATCH edk2 v2 3/3] StandaloneMmPkg: Fix check buffer address failed issue from TF-A To: =?UTF-8?Q?Marvin_H=c3=a4user?= , devel@edk2.groups.io, sami.mujawar@arm.com, ardb+tianocore@kernel.org, jiewen.yao@intel.com, supreeth.venkatesh@arm.com Cc: ming.huang-@outlook.com References: <20211221150618.113944-1-huangming@linux.alibaba.com> <20211221150618.113944-4-huangming@linux.alibaba.com> <2c1cb7ad-cc13-a9e6-ec6d-4e627393a30b@posteo.de> From: "Ming Huang" In-Reply-To: <2c1cb7ad-cc13-a9e6-ec6d-4e627393a30b@posteo.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 >> --- >>   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; >>