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.web09.63642.1640867256660788448 for ; Thu, 30 Dec 2021 04:27:37 -0800 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@posteo.de header.s=2017 header.b=OrD5FDG5; 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 0ACFA240105 for ; Thu, 30 Dec 2021 13:27:33 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1640867254; bh=uxjTVHh2YQrayYDbs0TtqxKzZZZzqeL8S10So12mO90=; h=Date:Subject:To:Cc:From:From; b=OrD5FDG5GGh7CcEYprz47b+y6JcTJTkg4KM9R+fzbyZyRFtjGocSNQb61TH8I5ST+ PaAjXTXY5MJrfp8oJjicpA7J6ZOIob6QVC/USYl50MFrK38Y7DVm8jfhqP6qnr5YKV iq6yWyl/1UFw/lI6MfrLitB5WivT19w7jIZ8pFovabYZ4te9do5c0joplOwOeteQSo jG/avx7ccVpO9/Ie4OHyZSM2WuXyRp+0E6v5iln25bUHqd0x1YbWv5HARHEBP7p8vi v4DhQo++y3joL7WRmjt2abecPeB4b/8SKqJT2r1mxrVxusCQT0N9MWgw0nXoxSDBUb POczG7teWLKXA== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4JPnbR6Qnyz9rxN; Thu, 30 Dec 2021 13:27:31 +0100 (CET) Message-ID: Date: Thu, 30 Dec 2021 12:27:28 +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: <898f19b9-a0eb-dfdb-0690-86cca1acb716@posteo.de> <8c4866d1-e312-4c80-0b39-37cc2a42c22a@linux.alibaba.com> <08c4b0e7-16ce-3562-feca-6b95a5443b00@linux.alibaba.com> From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= In-Reply-To: <08c4b0e7-16ce-3562-feca-6b95a5443b00@linux.alibaba.com> Content-Language: en-GB Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable On 25.12.21 03:09, Ming Huang wrote: > > =E5=9C=A8 12/24/21 9:52 PM, Marvin H=C3=A4user =E5=86=99=E9=81=93: >> On 24.12.21 02:18, Ming Huang wrote: >>> =E5=9C=A8 12/23/21 7:05 PM, Marvin H=C3=A4user =E5=86=99=E9=81=93: >>>> 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 = wrote: >>>>>>> >>>>>>> =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 i= ssue 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 addres= s. >>>>>>>>> This patch add CheckBufferAddr() to support check address for sec= ure buffer. >>>>>>>>> Signed-off-by: Ming Huang >>>>>>>>> --- >>>>>>>>> StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c=C2=A0=C2=A0= =C2=A0=C2=A0 | 70 >>>>>>>>> ++++++++++++++++---- >>>>>>>>> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21 >>>>>>>>> ++++++ >>>>>>>>> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h |=C2=A0= 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 w= ith >>>>>>>>> the normal world=C2=A0 EFI_MMRAM_DESCRIPTOR=C2=A0 mNsCommBuffer; >>>>>>>>> +EFI_MMRAM_DESCRIPTOR=C2=A0 mSCommBuffer; >>>>>>>>> MP_INFORMATION_HOB_DATA *mMpInformationHobData; >>>>>>>>> @@ -60,6 +61,58 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig =3D >>>>>>>>> { >>>>>>>>> STATIC EFI_MM_ENTRY_POINT=C2=A0=C2=A0=C2=A0=C2=A0 mMmEntryPoint = =3D NULL; >>>>>>>>> +STATIC >>>>>>>>> +EFI_STATUS >>>>>>>>> +CheckBufferAddr ( >>>>>>>>> +=C2=A0 IN UINTN CommBufferAddr >>>>>>>>> +=C2=A0 ) >>>>>>>>> +{ >>>>>>>>> +=C2=A0 UINTN=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 CommBufferSize; >>>>>>>>> +=C2=A0 EFI_STATUS Status; >>>>>>>>> + >>>>>>>>> +=C2=A0 Status =3D=C2=A0 EFI_SUCCESS; >>>>>>>>> +=C2=A0 if (CommBufferAddr < mNsCommBuffer.PhysicalStart) { >>>>>>>>> +=C2=A0=C2=A0=C2=A0 Status =3D EFI_ACCESS_DENIED; >>>>>>>>> +=C2=A0 } >>>>>>>>> + >>>>>>>>> +=C2=A0 if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER))= >=3D >>>>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (mNsCommBuffer.PhysicalStart + mN= sCommBuffer.PhysicalSize)) { >>>>>>>>> +=C2=A0=C2=A0=C2=A0 Status =3D=C2=A0 EFI_INVALID_PARAMETER; >>>>>>>> Single space after "Status =3D " >>>>>>> Modify it in v2. >>>>>>> >>>>>>>> - Omkar >>>>>>>>> +=C2=A0 } >>>>>>>>> + >>>>>>>>> +=C2=A0 // Find out the size of the buffer passed=C2=A0 CommBuffe= rSize =3D >>>>>>>>> + ((EFI_MM_COMMUNICATE_HEADER *) CommBufferAddr)->MessageLength >>>>>>>>> + >>>>>>>>> +=C2=A0=C2=A0=C2=A0 sizeof (EFI_MM_COMMUNICATE_HEADER); >>>>>>>>> + >>>>>>>>> +=C2=A0 // perform bounds check. >>>>>>>>> +=C2=A0 if (CommBufferAddr + CommBufferSize >=3D >>>>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mNsCommBuffer.PhysicalStart + mNs= CommBuffer.PhysicalSize) { >>>>>>>>> +=C2=A0=C2=A0=C2=A0 Status =3D=C2=A0 EFI_ACCESS_DENIED; >>>>>>>> Single space after "Status =3D " >>>>>>> Modify it in v2. >>>>>>> >>>>>>>> - Omkar >>>>>>>>> +=C2=A0 } >>>>>>>>> + >>>>>>>>> +=C2=A0 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 wit= h the Secure buffer address, which may cause wrong return type being return= ed. Can you check this, please? >>>>>>>> - Omkar >>>>>>>>> +=C2=A0=C2=A0=C2=A0 return EFI_SUCCESS; >>>>>>>>> +=C2=A0 } >>>>>>>>> + >>>>>>>>> +=C2=A0 Status =3D=C2=A0 EFI_SUCCESS; >>>>>>>>> +=C2=A0 if (CommBufferAddr < mSCommBuffer.PhysicalStart) { >>>>>>>>> +=C2=A0=C2=A0=C2=A0 Status =3D EFI_ACCESS_DENIED; >>>>>>>>> +=C2=A0 } >>>>>>>>> + >>>>>>>>> +=C2=A0 if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER))= >=3D >>>>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (mSCommBuffer.PhysicalStart + mSC= ommBuffer.PhysicalSize)) { >>>>>>>>> +=C2=A0=C2=A0=C2=A0 Status =3D=C2=A0 EFI_INVALID_PARAMETER; >>>>>>>>> +=C2=A0 } >>>>>>>>> + >>>>>>>>> +=C2=A0 // perform bounds check. >>>>>>>>> +=C2=A0 if (CommBufferAddr + CommBufferSize >=3D >>>>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mSCommBuffer.PhysicalStart + mSCo= mmBuffer.PhysicalSize) { >>>>>>>>> +=C2=A0=C2=A0=C2=A0 Status =3D=C2=A0 EFI_ACCESS_DENIED; >>>>>>>>> +=C2=A0 } >>>>>>>>> + >>>>>>>>> +=C2=A0 return Status; >>>>>>>>> +} >>>>>>>>> + >>>>>>>> CheckBufferAddr() function performs validity and overflow checks o= n the Communication buffers. These checks are same for both the non-secure >>>>>>>> MM communicate buffer and secure buffer shared between EL3 and S-E= L0. Can this code be combined ( example below)? This will help mitigate the= above mentioned return type issue as well. >>>>>>> Your example is a good idea to solve this case. I may modify it lik= e below in v2: >>>>>>> >>>>>>> STATIC >>>>>>> EFI_STATUS >>>>>>> CheckBufferAddr ( >>>>>>> IN UINTN CommBufferAddr >>>>>>> ) >>>>>>> { >>>>>>> UINTN=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 CommBufferSize; >>>>>>> EFI_STATUS Status; >>>>>>> UINT64=C2=A0=C2=A0=C2=A0=C2=A0 NsCommBufferEnd; >>>>>>> UINT64=C2=A0=C2=A0=C2=A0=C2=A0 SCommBufferEnd; >>>>>>> UINT64=C2=A0=C2=A0=C2=A0=C2=A0 CommBufferEnd; >>>>>>> >>>>>>> Status =3D=C2=A0 EFI_SUCCESS; >>>>>>> NsCommBufferEnd =3D mNsCommBuffer.PhysicalStart + mNsCommBuffer.Phy= sicalSize; >>>>>>> SCommBufferEnd =3D mSCommBuffer.PhysicalStart + mSCommBuffer.Physic= alSize; >>>>>>> >>>>>>> if ((CommBufferAddr >=3D mNsCommBuffer.PhysicalStart) && >>>>>>> (CommBufferAddr < NsCommBufferEnd)) { >>>>>>> CommBufferEnd =3D NsCommBufferEnd; >>>>>>> } else if ((CommBufferAddr >=3D mSCommBuffer.PhysicalStart) && >>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (CommBufferAddr <=3D SCommBuf= ferEnd)) { >>>>>> I find it odd the check here (lesser-equals) is inconsistent with th= e check above (lesser). It=E2=80=99d be caught below anyway, but I=E2=80=99= d change 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 Comm= BufferEnd) { >>>>>> Why is greater-equals used here? MessageLength =3D=3D 0 is not filte= red below, 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 thi= nk we do. We do know it holds that CommBufferAddr <=3D CommBufferEnd though= , so checking CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_H= EADER) would give you that for free, if we assume the UINT64 variables abov= e are actually bounded by UINTN, which seems reasonable - could ASSERT. >>>>> In my mind, (CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= =3D CommBufferEnd) >>>>> is the same with: CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COM= MUNICATE_HEADER) >>>> Okay, assume: >>>> CommBufferEnd =3D MAX_UINTN >>>> CommBufferAddr =3D MAX_UINTN - 1 (MAX_UINTN - 1 < MAX_UINTN, so the ch= eck above would pass) >>>> sizeof (EFI_MM_COMMUNICATE_HEADER) =3D 16 (should be accurate for 64-b= it architectures) >>>> >>>> Then (assume implicit mod 2^N on both sides due to bounded integers!): >>>> (CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=3D CommBufferE= nd) <=3D> ((MAX_UINTN - 1) + 16) >=3D MAX_UINTN <=3D> MAX_UINTN + 15 >=3D M= AX_UINTN <=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> MAX_UINTN - (MAX_UINTN - 1) < 16 <=3D> 1 < 16 <=3D> TRUE >>>> >>>> Due to wraparound semantics and the knowledge derived by the if-checks= above they are by no means the same. The left term of the first equation c= an wrap around (or it cannot, but then we need some concrete proof that it = cannot), and the left term of the second equation obviously cannot (directl= y follows from the if statements before). >>> I got it. Thanks for your proper comments. >>> I may modify it like below in v3: >>> ---------------------------------------- >>> STATIC >>> EFI_STATUS >>> CheckBufferAddr ( >>> =C2=A0=C2=A0 IN UINTN BufferAddr >>> =C2=A0=C2=A0 ) >>> { >>> =C2=A0=C2=A0 UINTN=C2=A0 BufferSize; >>> =C2=A0=C2=A0 UINT64 NsCommBufferEnd; >>> =C2=A0=C2=A0 UINT64 SCommBufferEnd; >>> =C2=A0=C2=A0 UINT64 CommBufferEnd; >>> >>> =C2=A0=C2=A0 NsCommBufferEnd =3D mNsCommBuffer.PhysicalStart + mNsComm= Buffer.PhysicalSize; >>> =C2=A0=C2=A0 SCommBufferEnd =3D mSCommBuffer.PhysicalStart + mSCommBuf= fer.PhysicalSize; >>> >>> =C2=A0=C2=A0 if ((BufferAddr >=3D mNsCommBuffer.PhysicalStart) && >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (BufferAddr < NsCommBufferEnd)) { >>> =C2=A0=C2=A0=C2=A0=C2=A0 CommBufferEnd =3D NsCommBufferEnd; >>> =C2=A0=C2=A0 } else if ((BufferAddr >=3D mSCommBuffer.PhysicalStart) &= & >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 (BufferAddr < SCommBufferEnd)) { >>> =C2=A0=C2=A0=C2=A0=C2=A0 CommBufferEnd =3D SCommBufferEnd; >>> =C2=A0=C2=A0 } else { >>> =C2=A0=C2=A0=C2=A0=C2=A0 return EFI_ACCESS_DENIED; >>> =C2=A0=C2=A0 } >>> >>> =C2=A0=C2=A0 if ((CommBufferEnd - BufferAddr) < sizeof (EFI_MM_COMMUNI= CATE_HEADER)) { >>> =C2=A0=C2=A0=C2=A0=C2=A0 return EFI_INVALID_PARAMETER; >> Just another cosmetic thing I just noticed, why is this Invalid Paramete= r? The check above yields Access Denied when the buffer start is out of a t= rusted range, OK. The check below yields Access Denied when the buffer data= extends beyond the trusted range, OK. What makes this check different from= the other ones that it gets a different return code? I'm not sure on the p= olicy of function documentation (i.e. whether one is needed), but what woul= d the difference be in their descriptions? > Okay, EFI_ACCESS_DENIED should be return. Modify it in v3. > >>> =C2=A0=C2=A0 } >>> >>> =C2=A0=C2=A0 // Find out the size of the buffer passed >>> =C2=A0=C2=A0 BufferSize =3D ((EFI_MM_COMMUNICATE_HEADER *) BufferAddr)= ->MessageLength + >>> =C2=A0=C2=A0=C2=A0=C2=A0 sizeof (EFI_MM_COMMUNICATE_HEADER); >> Same issue as above, can also be solved by rewriting as subtraction. Bec= ause you now know that (CommBufferEnd - BufferAddr) >=3D sizeof (EFI_MM_COM= MUNICATE_HEADER). > Sorry, I haven't understood what you mean. Could you rewrite CheckBufferA= ddr() as a sample? CommBufferEnd - BufferAddr - sizeof (EFI_MM_COMMUNICATE_HEADER) < ((EFI_MM_= COMMUNICATE_HEADER *) BufferAddr)->MessageLength Best regards, Marvin > > Thank you very much. > Merry Christmas! > > - Ming > >> Best regards, >> Marvin >> >>> =C2=A0=C2=A0 // perform bounds check. >>> =C2=A0=C2=A0 if ((CommBufferEnd - BufferAddr) < BufferSize) { >>> =C2=A0=C2=A0=C2=A0=C2=A0 return EFI_ACCESS_DENIED; >>> =C2=A0=C2=A0 } >>> >>> =C2=A0=C2=A0 return EFI_SUCCESS; >>> } >>> ---------------------------------------- >>> >>> - Ming >>> >>>>>> Alternatively, you could not store the maximum buffer end but the ma= ximum 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=C2=A0 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)->= MessageLength + >>>>>>> sizeof (EFI_MM_COMMUNICATE_HEADER); >>>>>> Same wraparound concern, same suggestion for solving it. >>>>>> >>>>>>> // perform bounds check. >>>>>>> if (CommBufferAddr + CommBufferSize >=3D CommBufferEnd) { >>>>>>> Status =3D=C2=A0 EFI_ACCESS_DENIED; >>>>>> It=E2=80=99s obviously not bad here, but for consistency=E2=80=99s s= ake, to mitigate bugs introduced by future changes, and readability, I=E2= =80=99d return here and just return EFI_SUCCESS below, removing the code re= quirement 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 com= munication buffer. Buffer validity checking is too critical than to duplica= te it in every consumer. >>>>>> >>>>>> Thanks! >>>>>> >>>>>> Best regards, >>>>>> Marvin >>>>>> >>>>>>> } >>>>>>> >>>>>>> return Status; >>>>>>> } >>>>>>> >>>>>>> - Ming >>>>>>> >>>>>>>> STATIC >>>>>>>> EFI_STATUS >>>>>>>> CheckBufferAddr ( >>>>>>>> IN UINTN CommBufferAddr >>>>>>>> ) >>>>>>>> { >>>>>>>> UINTN=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 CommBufferSize; >>>>>>>> EFI_STATUS=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 Status; >>>>>>>> EFI_MMRAM_DESCRIPTOR CommBuffer; >>>>>>>> if (CommBufferAddr < mNsCommBuffer.PhysicalStart || >>>>>>>> CommBufferAddr > (mNsCommBuffer.PhysicalStart + mNsCommBuffer.Phys= icalSize)) { >>>>>>>> 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)-= >MessageLength + >>>>>>>> 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; >>>>>>>>> } >>>>>>>>> -=C2=A0 if (NsCommBufferAddr < mNsCommBuffer.PhysicalStart) { >>>>>>>>> -=C2=A0=C2=A0=C2=A0 return EFI_ACCESS_DENIED; >>>>>>>>> -=C2=A0 } >>>>>>>>> - >>>>>>>>> -=C2=A0 if ((NsCommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER= )) >=3D >>>>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (mNsCommBuffer.PhysicalStart + mN= sCommBuffer.PhysicalSize)) { >>>>>>>>> -=C2=A0=C2=A0=C2=A0 return EFI_INVALID_PARAMETER; >>>>>>>>> +=C2=A0 Status =3D CheckBufferAddr (NsCommBufferAddr);=C2=A0 if (= EFI_ERROR (Status)) >>>>>>>>> + { >>>>>>>>> +=C2=A0=C2=A0=C2=A0 DEBUG ((DEBUG_ERROR, "Check Buffer failed: %r= \n", Status)); >>>>>>>>> +=C2=A0=C2=A0=C2=A0 return Status; >>>>>>>>> } >>>>>>>>> // Find out the size of the buffer passed >>>>>>>>> NsCommBufferSize =3D ((EFI_MM_COMMUNICATE_HEADER *) >>>>>>>>> NsCommBufferAddr)->MessageLength + >>>>>>>>> sizeof (EFI_MM_COMMUNICATE_HEADER); >>>>>>>>> -=C2=A0 // perform bounds check. >>>>>>>>> -=C2=A0 if (NsCommBufferAddr + NsCommBufferSize >=3D >>>>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mNsCommBuffer.PhysicalStart + mNs= CommBuffer.PhysicalSize) { >>>>>>>>> -=C2=A0=C2=A0=C2=A0 return EFI_ACCESS_DENIED; >>>>>>>>> -=C2=A0 } >>>>>>>>> - >>>>>>>>> GuidedEventContext =3D NULL; >>>>>>>>> // Now that the secure world can see the normal world buffer, all= ocate >>>>>>>>> // 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=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 Index; >>>>>>>>> UINTN=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 ArraySize; >>>>>>>>> VOID=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 *HobStart; >>>>>>>>> +=C2=A0 EFI_MMRAM_HOB_DESCRIPTOR_BLOCK=C2=A0 *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)); >>>>>>>>> +=C2=A0 Status =3D GetGuidedHobData ( >>>>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 HobStart, >>>>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 &gEfiMmPeiMmramMemoryReserveGuid, >>>>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 (VOID **) &MmramRangesHob >>>>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 ); >>>>>>>>> +=C2=A0 if (EFI_ERROR (Status)) { >>>>>>>>> +=C2=A0=C2=A0=C2=A0 DEBUG ((DEBUG_ERROR, "MmramRangesHob data ext= raction failed - >>>>>>>>> 0x%x\n", Status)); >>>>>>>>> +=C2=A0=C2=A0=C2=A0 return Status; >>>>>>>>> +=C2=A0 } >>>>>>>>> + >>>>>>>>> +=C2=A0 // >>>>>>>>> +=C2=A0 // As CreateHobListFromBootInfo(), the base and size of b= uffer shared >>>>>>>>> + with=C2=A0 // privileged Secure world software is in second one= . >>>>>>>>> +=C2=A0 // >>>>>>>>> +=C2=A0 CopyMem ( >>>>>>>>> +=C2=A0=C2=A0=C2=A0 &mSCommBuffer, >>>>>>>>> +=C2=A0=C2=A0=C2=A0 &MmramRangesHob->Descriptor[0] + 1, >>>>>>>> Can this be changed to >>>>>>>> &MmramRangesHob->Descriptor[1], >>>>>>>> - Omkar >>>>>>>>> +=C2=A0=C2=A0=C2=A0 sizeof(EFI_MMRAM_DESCRIPTOR) >>>>>>>>> +=C2=A0 ); >>>>>>>>> + >>>>>>>>> // >>>>>>>>> // 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;=C2=A0 /= / >>>>>>>>> extern EFI_MM_COMMUNICATE_HEADER=C2=A0=C2=A0=C2=A0 **PerCpuGuided= EventContext; >>>>>>>>> extern EFI_MMRAM_DESCRIPTOR=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 mNsCommBuffer; >>>>>>>>> +extern EFI_MMRAM_DESCRIPTOR=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 mSCommBuffer; >>>>>>>>> extern MP_INFORMATION_HOB_DATA=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 *mMpInformationHobData; >>>>>>>>> extern EFI_MM_CONFIGURATION_PROTOCOL mMmConfig; >>>>>>>>> --=20 >>>>>>>>> 2.17.1 >>>>>>>> IMPORTANT NOTICE: The contents of this email and any attachments a= re confidential and may also be privileged. If you are not the intended rec= ipient, please notify the sender immediately and do not disclose the conten= ts to any other person, use it for any purpose, or store or copy the inform= ation in any medium. Thank you. >>>>>>>=20