From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out199-3.us.a.mail.aliyun.com (out199-3.us.a.mail.aliyun.com [47.90.199.3]) by mx.groups.io with SMTP id smtpd.web11.6252.1652433690774956827 for ; Fri, 13 May 2022 02:21:31 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: linux.alibaba.com, ip: 47.90.199.3, mailfrom: huangming@linux.alibaba.com) X-Alimail-AntiSpam: AC=PASS;BC=-1|-1;BR=01201311R151e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=e01e04400;MF=huangming@linux.alibaba.com;NM=1;PH=DS;RN=7;SR=0;TI=SMTPD_---0VD2dY16_1652433684; Received: from 30.18.74.38(mailfrom:huangming@linux.alibaba.com fp:SMTPD_---0VD2dY16_1652433684) by smtp.aliyun-inc.com(127.0.0.1); Fri, 13 May 2022 17:21:25 +0800 Message-ID: <89f516c9-fa8d-3f33-d7e1-4c3799ffaebb@linux.alibaba.com> Date: Fri, 13 May 2022 17:21:24 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: Re: [edk2-devel] [PATCH edk2 v3 3/3] StandaloneMmPkg: Fix check buffer address failed issue from TF-A To: devel@edk2.groups.io, sami.mujawar@arm.com, "ardb+tianocore@kernel.org" , "jiewen.yao@intel.com" , Supreeth Venkatesh Cc: "ming.huang-@outlook.com" , nd References: <20211231110623.128888-1-huangming@linux.alibaba.com> <20211231110623.128888-4-huangming@linux.alibaba.com> <7a415d5e-701c-266d-3d0e-1617a31bc050@linux.alibaba.com> <843FF6EA-3E67-4802-B562-83FB7ADA823E@arm.com> From: "Ming Huang" In-Reply-To: <843FF6EA-3E67-4802-B562-83FB7ADA823E@arm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hi Sami, Sorry for the late reply. https://github.com/waip23/edk2/tree/upstread-fix-mm-issue branch: upstread-fix-mm-issue Regards, Ming Huang 在 4/6/22 5:03 PM, Sami Mujawar 写道: > Hi Ming, > > I am not sure if this is an issue at my end, but I cannot apply this patch series, can you check, please? > Also, is it possible to share these patches on a Github branch. > > Regards, > > Sami Mujawar > > On 30/03/2022, 10:37, "Ming Huang" wrote: > > Hi, > > Any comment about this series? > > 在 12/31/21 7:06 PM, Ming Huang 写道: > > 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 | 54 +++++++++++++++----- > > StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21 ++++++++ > > StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h | 1 + > > 3 files changed, 63 insertions(+), 13 deletions(-) > > > > diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c > > index 5dfaf9d751..d0ba415671 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,42 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig = { > > > > STATIC EFI_MM_ENTRY_POINT mMmEntryPoint = NULL; > > > > +STATIC > > +EFI_STATUS > > +CheckBufferAddr ( > > + IN UINTN BufferAddr > > + ) > > +{ > > + UINT64 NsCommBufferEnd; > > + UINT64 SCommBufferEnd; > > + UINT64 CommBufferEnd; > > + > > + NsCommBufferEnd = mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize; > > + SCommBufferEnd = mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize; > > + > > + if ((BufferAddr >= mNsCommBuffer.PhysicalStart) && > > + (BufferAddr < NsCommBufferEnd)) { > > + CommBufferEnd = NsCommBufferEnd; > > + } else if ((BufferAddr >= mSCommBuffer.PhysicalStart) && > > + (BufferAddr < SCommBufferEnd)) { > > + CommBufferEnd = SCommBufferEnd; > > + } else { > > + return EFI_ACCESS_DENIED; > > + } > > + > > + if ((CommBufferEnd - BufferAddr) < sizeof (EFI_MM_COMMUNICATE_HEADER)) { > > + return EFI_ACCESS_DENIED; > > + } > > + > > + // perform bounds check. > > + if ((CommBufferEnd - BufferAddr - sizeof (EFI_MM_COMMUNICATE_HEADER)) < > > + ((EFI_MM_COMMUNICATE_HEADER *) BufferAddr)->MessageLength) { > > + return EFI_ACCESS_DENIED; > > + } > > + > > + return EFI_SUCCESS; > > +} > > + > > /** > > The PI Standalone MM entry point for the TF-A CPU driver. > > > > @@ -104,25 +141,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; > > > > > > > >