From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout02.posteo.de (mout02.posteo.de [185.67.36.66]) by mx.groups.io with SMTP id smtpd.web08.8307.1639646298912727404 for ; Thu, 16 Dec 2021 01:18:19 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=E2I7va9z; spf=pass (domain: posteo.de, ip: 185.67.36.66, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [89.146.220.130]) by mout02.posteo.de (Postfix) with ESMTPS id 288CE240107 for ; Thu, 16 Dec 2021 10:18:10 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1639646291; bh=rW3uGZ/EvFy6svktuBPoBGZtb38vi0zNy2iAI6v0vcY=; h=From:Subject:Date:To:Cc:From; b=E2I7va9zf+rGdSp7rI/U6syg9wECxzSh8asznWUkCfdrATQU1c54q+wr3FK4alp6y VGdlVGfYSSxEpl1Riku6Dqv2mHrPbWU8IUa7h+EMG5nCLD4+NgemBx+3LsDcMmdLZx RQkny6AYB8VsvpPrao3m+vgCs5Uzy5z0Uwj7RbybLbEcN1DBQ0CIxdPMwMkL63Jz+K YgoMKCbHs6EqC6BHYWTFVjuLeNzEBHbj0mZgTOT0NgezlSVAY619axsttuyMUdg7sD l7mRosDTYYiuDay6Jl2qBNptAHq+Zlu65G66uXEedaBhLiklmBvQayH9VtwygDavY3 wzjjhrL2XIuHw== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4JF63N6WtPz6tqQ; Thu, 16 Dec 2021 10:18:08 +0100 (CET) From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Mime-Version: 1.0 (1.0) Subject: [edk2-devel] [PATCH edk2 v1 3/3] StandaloneMmPkg: Fix check buffer address failed issue from TF-A Message-Id: Date: Thu, 16 Dec 2021 09:15:49 +0000 To: devel@edk2.groups.io, huangming@linux.alibaba.com Cc: omkar.kulkarni@arm.com, Sami Mujawar , ardb+tianocore@kernel.org, jiewen.yao@intel.com, Supreeth Venkatesh , ming.huang-@outlook.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable =EF=BB=BFHey all, > On 15. Dec 2021, at 16:02, Ming Huang wrote= : >=20 > =EF=BB=BF >=20 >> 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 w= hile 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 bu= ffer. >>> 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 =3D 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 =3D >>> { >>> STATIC EFI_MM_ENTRY_POINT mMmEntryPoint =3D NULL; >>> +STATIC >>> +EFI_STATUS >>> +CheckBufferAddr ( >>> + IN UINTN CommBufferAddr >>> + ) >>> +{ >>> + UINTN CommBufferSize; >>> + EFI_STATUS Status; >>> + >>> + Status =3D EFI_SUCCESS; >>> + if (CommBufferAddr < mNsCommBuffer.PhysicalStart) { >>> + Status =3D EFI_ACCESS_DENIED; >>> + } >>> + >>> + if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=3D >>> + (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) { >>> + Status =3D EFI_INVALID_PARAMETER; >> Single space after "Status =3D " >=20 > Modify it in v2. >=20 >> - Omkar >>> + } >>> + >>> + // Find out the size of the buffer passed CommBufferSize =3D >>> + ((EFI_MM_COMMUNICATE_HEADER *) CommBufferAddr)->MessageLength >>> + >>> + sizeof (EFI_MM_COMMUNICATE_HEADER); >>> + >>> + // perform bounds check. >>> + if (CommBufferAddr + CommBufferSize >=3D >>> + mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) { >>> + Status =3D EFI_ACCESS_DENIED; >> Single space after "Status =3D " >=20 > Modify it in v2. >=20 >> - Omkar >>> + } >>> + >>> + if (!EFI_ERROR (Status)) { >> In case of error this function call will not return from here. It will e= xecute the code below comparing the MM Communicate buffer address with the = Secure buffer address, which may cause wrong return type being returned. Ca= n you check this, please? >> - Omkar >>> + return EFI_SUCCESS; >>> + } >>> + >>> + Status =3D EFI_SUCCESS; >>> + if (CommBufferAddr < mSCommBuffer.PhysicalStart) { >>> + Status =3D EFI_ACCESS_DENIED; >>> + } >>> + >>> + if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=3D >>> + (mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize)) { >>> + Status =3D EFI_INVALID_PARAMETER; >>> + } >>> + >>> + // perform bounds check. >>> + if (CommBufferAddr + CommBufferSize >=3D >>> + mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize) { >>> + Status =3D 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. Ca= n this code be combined ( example below)? This will help mitigate the above= mentioned return type issue as well. >=20 > Your example is a good idea to solve this case. I may modify it like belo= w in v2: >=20 > STATIC > EFI_STATUS > CheckBufferAddr ( > IN UINTN CommBufferAddr > ) > { > UINTN CommBufferSize; > EFI_STATUS Status; > UINT64 NsCommBufferEnd; > UINT64 SCommBufferEnd; > UINT64 CommBufferEnd; >=20 > Status =3D EFI_SUCCESS; > NsCommBufferEnd =3D mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalS= ize; > SCommBufferEnd =3D mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize= ; >=20 > if ((CommBufferAddr >=3D mNsCommBuffer.PhysicalStart) && > (CommBufferAddr < NsCommBufferEnd)) { > CommBufferEnd =3D NsCommBufferEnd; > } else if ((CommBufferAddr >=3D mSCommBuffer.PhysicalStart) && > (CommBufferAddr <=3D SCommBufferEnd)) { I find it odd the check here (lesser-equals) is inconsistent with the check= above (lesser). It=E2=80=99d be caught below anyway, but I=E2=80=99d chang= e this to lesser to keep the return codes consistent. > CommBufferEnd =3D SCommBufferEnd; > } else { > return EFI_ACCESS_DENIED; > } >=20 > if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=3D CommBuffer= End) { Why is greater-equals used here? MessageLength =3D=3D 0 is not filtered bel= ow, 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=E2=80=99t think we d= o. We do know it holds that CommBufferAddr <=3D CommBufferEnd though, so ch= ecking CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER) = would give you that for free, if we assume the UINT64 variables above are a= ctually bounded by UINTN, which seems reasonable - could ASSERT. Alternatively, you could not store the maximum buffer end but the maximum b= uffer size, so the additions of the buffer start would just vanish. This mi= ght be more readable too I think. > Status =3D EFI_INVALID_PARAMETER; Why is there no return here? This can proceed when the buffer cannot fit th= is header, and yet below the header is dereferenced. > } >=20 > // Find out the size of the buffer passed > CommBufferSize =3D ((EFI_MM_COMMUNICATE_HEADER *) CommBufferAddr)->Messag= eLength + > sizeof (EFI_MM_COMMUNICATE_HEADER); Same wraparound concern, same suggestion for solving it. > // perform bounds check. > if (CommBufferAddr + CommBufferSize >=3D CommBufferEnd) { > Status =3D EFI_ACCESS_DENIED; It=E2=80=99s obviously not bad here, but for consistency=E2=80=99s sake, to= mitigate bugs introduced by future changes, and readability, I=E2=80=99d r= eturn 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 w= ay that it can be consumed by all places that accept any sort of communicat= ion buffer. Buffer validity checking is too critical than to duplicate it i= n every consumer. Thanks! Best regards, Marvin > } >=20 > return Status; > } >=20 > - Ming >=20 >> STATIC >> EFI_STATUS >> CheckBufferAddr ( >> IN UINTN CommBufferAddr >> ) >> { >> UINTN CommBufferSize; >> EFI_STATUS Status; >> EFI_MMRAM_DESCRIPTOR CommBuffer; >> if (CommBufferAddr < mNsCommBuffer.PhysicalStart || >> CommBufferAddr > (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSi= ze)) { >> CommBuffer =3D mSCommBuffer; >> } else { >> CommBuffer =3D mNsCommBuffer; >> } >> if (CommBufferAddr < CommBuffer.PhysicalStart) { >> Status =3D EFI_ACCESS_DENIED; >> } >> if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=3D >> (CommBuffer.PhysicalStart + CommBuffer.PhysicalSize)) { >> Status =3D EFI_INVALID_PARAMETER; >> } >> // Find out the size of the buffer passed >> CommBufferSize =3D ((EFI_MM_COMMUNICATE_HEADER *) CommBufferAddr)->Messa= geLength + >> sizeof (EFI_MM_COMMUNICATE_HEADER); >> // perform bounds check. >> if (CommBufferAddr + CommBufferSize >=3D >> CommBuffer.PhysicalStart + CommBuffer.PhysicalSize) { >> Status =3D 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)) >=3D >>> - (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) { >>> - return EFI_INVALID_PARAMETER; >>> + Status =3D CheckBufferAddr (NsCommBufferAddr); if (EFI_ERROR (Statu= s)) >>> + { >>> + DEBUG ((DEBUG_ERROR, "Check Buffer failed: %r\n", Status)); >>> + return Status; >>> } >>> // Find out the size of the buffer passed >>> NsCommBufferSize =3D ((EFI_MM_COMMUNICATE_HEADER *) >>> NsCommBufferAddr)->MessageLength + >>> sizeof (EFI_MM_COMMUNICATE_HEADER); >>> - // perform bounds check. >>> - if (NsCommBufferAddr + NsCommBufferSize >=3D >>> - mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) { >>> - return EFI_ACCESS_DENIED; >>> - } >>> - >>> GuidedEventContext =3D 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 !=3D NULL); >>> mMmst =3D 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 =3D 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 share= d >>> + 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 con= fidential 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. >=20 >=20 >=20