From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out30-131.freemail.mail.aliyun.com (out30-131.freemail.mail.aliyun.com [115.124.30.131]) by mx.groups.io with SMTP id smtpd.web12.6690.1640098797509802376 for ; Tue, 21 Dec 2021 07:00:00 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: linux.alibaba.com, ip: 115.124.30.131, mailfrom: huangming@linux.alibaba.com) X-Alimail-AntiSpam: AC=PASS;BC=-1|-1;BR=01201311R541e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=e01e04426;MF=huangming@linux.alibaba.com;NM=1;PH=DS;RN=7;SR=0;TI=SMTPD_---0V.LE1q3_1640098789; Received: from 30.15.216.18(mailfrom:huangming@linux.alibaba.com fp:SMTPD_---0V.LE1q3_1640098789) by smtp.aliyun-inc.com(127.0.0.1); Tue, 21 Dec 2021 22:59:50 +0800 Message-ID: <35a1d9fe-b027-6638-5afc-db389a8007cb@linux.alibaba.com> Date: Tue, 21 Dec 2021 22:59:49 +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.0 Subject: Re: [edk2-devel] [PATCH edk2 v1 3/3] StandaloneMmPkg: Fix check buffer address failed issue from TF-A To: devel@edk2.groups.io, omkar.kulkarni@arm.com, Sami Mujawar , "ardb+tianocore@kernel.org" , "jiewen.yao@intel.com" , Supreeth Venkatesh Cc: "ming.huang-@outlook.com" References: <20211015090623.52511-1-huangming@linux.alibaba.com> <20211015090623.52511-4-huangming@linux.alibaba.com> From: "Ming Huang" In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 在 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 >> --- >> 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. > > > >