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.web09.31609.1640257550821157927 for ; Thu, 23 Dec 2021 03:05:51 -0800 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@posteo.de header.s=2017 header.b=PGI7XWrc; 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 1044C24002B for ; Thu, 23 Dec 2021 12:05:48 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1640257549; bh=dKaFc8L8M7o3X/2zdcOSpR4L5jaAR8RvsXicorDs00Q=; h=Date:Subject:To:Cc:From:From; b=PGI7XWrcVkN3+LC0VMGxMEeQ5ZafPGmiutVP3yoJ0+/yHdA0/lak+XjteD4rgAPYs G0z5Fr0gWOuUFZurhaxULEW+CvJ/EYd2bjoaDOt/16l0cN9HfN71CEgX45KsXBwUYB qKkT/cd6ffkJUYfA6J02Cy+5BqPXY1iDcDZf6TgYcJnVCf6hokwdx1C+H6on2WFsEn +QWfnv+4h24AUwfE+ZFoetmj0dv2QlRADfr9wGz9mnHZU5x0aE486nO98HIq4xI+pu /CajcoFHvqLgsAkRHRvO0SNqZwX4pWjRXKh3n/WsnwpY6+a8lSgsNkgUKW3e0R9quw nUja5zy+8/ynw== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4JKS6J2fRfz6tnY; Thu, 23 Dec 2021 12:05:43 +0100 (CET) Message-ID: <898f19b9-a0eb-dfdb-0690-86cca1acb716@posteo.de> Date: Thu, 23 Dec 2021 11:05:43 +0000 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH edk2 v1 3/3] StandaloneMmPkg: Fix check buffer address failed issue from TF-A To: Ming Huang , devel@edk2.groups.io Cc: omkar.kulkarni@arm.com, Sami Mujawar , ardb+tianocore@kernel.org, jiewen.yao@intel.com, Supreeth Venkatesh , ming.huang-@outlook.com References: From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= In-Reply-To: Content-Language: en-GB Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable On 23.12.21 11:46, Ming Huang wrote: > > =E5=9C=A8 12/16/21 5:15 PM, Marvin H=C3=A4user =E5=86=99=E9=81=93: >> =EF=BB=BFHey all, >> >>> On 15. Dec 2021, at 16:02, Ming Huang wro= te: >>> >>> =EF=BB=BF >>> >>>> 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= 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 =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 " >>> Modify it in v2. >>> >>>> - 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 " >>> Modify it in v2. >>> >>>> - 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 th= e Secure buffer address, which may cause wrong return type being returned. = Can 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 th= e 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 abo= ve mentioned return type issue as well. >>> Your example is a good idea to solve this case. I may modify it like be= low in v2: >>> >>> STATIC >>> EFI_STATUS >>> CheckBufferAddr ( >>> IN UINTN CommBufferAddr >>> ) >>> { >>> UINTN CommBufferSize; >>> EFI_STATUS Status; >>> UINT64 NsCommBufferEnd; >>> UINT64 SCommBufferEnd; >>> UINT64 CommBufferEnd; >>> >>> Status =3D EFI_SUCCESS; >>> NsCommBufferEnd =3D mNsCommBuffer.PhysicalStart + mNsCommBuffer.Physica= lSize; >>> SCommBufferEnd =3D mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSi= ze; >>> >>> 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 ch= eck above (lesser). It=E2=80=99d be caught below anyway, but I=E2=80=99d ch= ange this to lesser to keep the return codes consistent. > Should be lesser, modify it in v3. > >>> CommBufferEnd =3D SCommBufferEnd; >>> } else { >>> return EFI_ACCESS_DENIED; >>> } >>> >>> if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=3D CommBuff= erEnd) { >> Why is greater-equals used here? MessageLength =3D=3D 0 is not filtered = below, so this looks odd to be honest, as this is only the theoretical maxi= mum buffer end. >> >> How do you know this cannot wraparound? I actually don=E2=80=99t think w= e do. We do know it holds that CommBufferAddr <=3D CommBufferEnd though, so= checking CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADE= R) would give you that for free, if we assume the UINT64 variables above ar= e actually bounded by UINTN, which seems reasonable - could ASSERT. > In my mind, (CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=3D Co= mmBufferEnd) > is the same with: CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNI= CATE_HEADER) Okay, assume: CommBufferEnd =3D MAX_UINTN CommBufferAddr =3D MAX_UINTN - 1 (MAX_UINTN - 1 < MAX_UINTN, so the check= =20 above would pass) sizeof (EFI_MM_COMMUNICATE_HEADER) =3D 16 (should be accurate for 64-bit=20 architectures) Then (assume implicit mod 2^N on both sides due to bounded integers!): (CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=3D CommBufferEnd)= =20 <=3D> ((MAX_UINTN - 1) + 16) >=3D MAX_UINTN <=3D> MAX_UINTN + 15 >=3D MAX_U= INTN=20 <=3D>(wraparound!!) 14 >=3D MAX_UINTN <=3D> FALSE And (assume implicit mod 2^N on both sides due to bounded integers!): CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER) <=3D>= =20 MAX_UINTN - (MAX_UINTN - 1) < 16 <=3D> 1 < 16 <=3D> TRUE Due to wraparound semantics and the knowledge derived by the if-checks=20 above they are by no means the same. The left term of the first equation=20 can wrap around (or it cannot, but then we need some concrete proof that=20 it cannot), and the left term of the second equation obviously cannot=20 (directly follows from the if statements before). >> Alternatively, you could not store the maximum buffer end but the maximu= m buffer size, so the additions of the buffer start would just vanish. This= might be more readable too I think. > As CommBufferAddr may be not at the begin of communicate buffer, > so check size with the maximum buffer size is not enough. > >>> Status =3D EFI_INVALID_PARAMETER; >> Why is there no return here? This can proceed when the buffer cannot fit= this header, and yet below the header is dereferenced. > Modify it in v3. > >>> } >>> >>> // Find out the size of the buffer passed >>> CommBufferSize =3D ((EFI_MM_COMMUNICATE_HEADER *) CommBufferAddr)->Mess= ageLength + >>> 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=99= d return here and just return EFI_SUCCESS below, removing the code requirem= ent of keeping Status consistent with the check results. > Modify it in v3. Thanks for the modifications. Best regards, Marvin > > - Ming > >> Finally, I really believe this kind of function should be abstracted in = a way that it can be consumed by all places that accept any sort of communi= cation buffer. Buffer validity checking is too critical than to duplicate i= t in every consumer. >> >> Thanks! >> >> Best regards, >> Marvin >> >>> } >>> >>> return Status; >>> } >>> >>> - Ming >>> >>>> STATIC >>>> EFI_STATUS >>>> CheckBufferAddr ( >>>> IN UINTN CommBufferAddr >>>> ) >>>> { >>>> UINTN CommBufferSize; >>>> EFI_STATUS Status; >>>> EFI_MMRAM_DESCRIPTOR CommBuffer; >>>> if (CommBufferAddr < mNsCommBuffer.PhysicalStart || >>>> CommBufferAddr > (mNsCommBuffer.PhysicalStart + mNsCommBuffer.Physical= Size)) { >>>> 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)->Mes= sageLength + >>>> 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 (Sta= tus)) >>>>> + { >>>>> + 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, allocat= e >>>>> // 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 sha= red >>>>> + 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 c= onfidential and may also be privileged. If you are not the intended recipie= nt, please notify the sender immediately and do not disclose the contents t= o any other person, use it for any purpose, or store or copy the informatio= n in any medium. Thank you. >>> >>>=20