From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) by mx.groups.io with SMTP id smtpd.web08.18805.1640178067151200709 for ; Wed, 22 Dec 2021 05:01:08 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=sBRY9Xhq; spf=pass (domain: posteo.de, ip: 185.67.36.65, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [89.146.220.130]) by mout01.posteo.de (Postfix) with ESMTPS id F2FA124002A for ; Wed, 22 Dec 2021 14:01:04 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1640178065; bh=INol5JAvPUO4TnI+fwvfN6kbUVniYd5Ri92L5k4Vj04=; h=Date:Subject:To:Cc:From:From; b=sBRY9Xhqeu0W8uFjUVhlcVproZ9kjf3EpjJk+PN8xZEJ2IW48HW2g8LHhG933vs6z kp02UZYTDLRoNa+/MfC8VzKX+xDrxXQtCw3ne+crwl06Tt14XNnzhNtpxnXdzXvg6F NPzM3NrTYt7mhWNcbFB0+EndUpbZ3S/EZrz8hGqXfmeenVuPz+gyb4XG3n0PsnLTiF W3VZd73SH4wdGuM5jsij3SiSIj7cEGfhZj3d6gBEVPWwoWH2+apJKdCQnww4Yk/U78 sr3Y+NqOT+KOPq1iA3FJey94LmYViWjyL6/W1UvVZ4pbn+lXuEiSf99w/kz/k07sIv pCU6Q530dSoDA== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4JJtjp6SLXz6tlh; Wed, 22 Dec 2021 14:01:02 +0100 (CET) Message-ID: <2c1cb7ad-cc13-a9e6-ec6d-4e627393a30b@posteo.de> Date: Wed, 22 Dec 2021 13:01:02 +0000 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH edk2 v2 3/3] StandaloneMmPkg: Fix check buffer address failed issue from TF-A To: devel@edk2.groups.io, huangming@linux.alibaba.com, 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> From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= In-Reply-To: <20211221150618.113944-4-huangming@linux.alibaba.com> Content-Language: en-GB Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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; >