在 12/16/21 5:15 PM, Marvin Häuser 写道:
> Hey all,
>
>> On 15. Dec 2021, at 16:02, Ming Huang <huangm...@linux.alibaba.com> wrote:
>>
>>
>>
>>> 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 <huangm...@linux.alibaba.com>
>>>> ---
>>>> 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 = 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 =
>>>> {
>>>> STATIC EFI_MM_ENTRY_POINT mMmEntryPoint = NULL;
>>>> +STATIC
>>>> +EFI_STATUS
>>>> +CheckBufferAddr (
>>>> + IN UINTN CommBufferAddr
>>>> + )
>>>> +{
>>>> + UINTN CommBufferSize;
>>>> + EFI_STATUS Status;
>>>> +
>>>> + Status = EFI_SUCCESS;
>>>> + if (CommBufferAddr < mNsCommBuffer.PhysicalStart) {
>>>> + Status = EFI_ACCESS_DENIED;
>>>> + }
>>>> +
>>>> + if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>> + (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>>>> + Status = EFI_INVALID_PARAMETER;
>>> Single space after "Status = "
>>
>> Modify it in v2.
>>
>>> - Omkar
>>>> + }
>>>> +
>>>> + // 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 >=
>>>> + mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
>>>> + Status = EFI_ACCESS_DENIED;
>>> Single space after "Status = "
>>
>> 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 the
>>> Secure buffer address, which may cause wrong return type being returned.
>>> Can you check this, please?
>>> - Omkar
>>>> + return EFI_SUCCESS;
>>>> + }
>>>> +
>>>> + Status = EFI_SUCCESS;
>>>> + if (CommBufferAddr < mSCommBuffer.PhysicalStart) {
>>>> + Status = EFI_ACCESS_DENIED;
>>>> + }
>>>> +
>>>> + if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>> + (mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize)) {
>>>> + Status = EFI_INVALID_PARAMETER;
>>>> + }
>>>> +
>>>> + // perform bounds check.
>>>> + if (CommBufferAddr + CommBufferSize >=
>>>> + mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize) {
>>>> + Status = 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. 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 like below
>> in v2:
>>
>> 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)) {
>
> I find it odd the check here (lesser-equals) is inconsistent with the check
> above (lesser). It’d be caught below anyway, but I’d change this to lesser to
> keep the return codes consistent.
Should be lesser, modify it in v3.
>
>> CommBufferEnd = SCommBufferEnd;
>> } else {
>> return EFI_ACCESS_DENIED;
>> }
>>
>> if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd) {
>
> Why is greater-equals used here? MessageLength == 0 is not filtered 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’t think we do. We do
> know it holds that CommBufferAddr <= CommBufferEnd though, so checking
> CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER) would
> give you that for free, if we assume the UINT64 variables above are actually
> bounded by UINTN, which seems reasonable - could ASSERT.
In my mind, (CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
CommBufferEnd)
is the same with: CommBufferEnd - CommBufferAddr < sizeof
(EFI_MM_COMMUNICATE_HEADER)
>
> Alternatively, you could not store the maximum buffer end but the maximum
> 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 = 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 = ((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 >= CommBufferEnd) {
>> Status = EFI_ACCESS_DENIED;
>
> It’s obviously not bad here, but for consistency’s sake, to mitigate bugs
> introduced by future changes, and readability, I’d return here and just
> return EFI_SUCCESS below, removing the code requirement of keeping Status
> consistent with the check results.
Modify it in v3.
- 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 communication
> buffer. Buffer validity checking is too critical than to duplicate it 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.PhysicalSize)) {
>>> CommBuffer = mSCommBuffer;
>>> } else {
>>> CommBuffer = mNsCommBuffer;
>>> }
>>> if (CommBufferAddr < CommBuffer.PhysicalStart) {
>>> Status = EFI_ACCESS_DENIED;
>>> }
>>> if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>> (CommBuffer.PhysicalStart + CommBuffer.PhysicalSize)) {
>>> 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 >=
>>> CommBuffer.PhysicalStart + CommBuffer.PhysicalSize) {
>>> Status = 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)) >=
>>>> - (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,
>>> 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
>>> confidential 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.
>>
>>
>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#85200): https://edk2.groups.io/g/devel/message/85200
Mute This Topic: https://groups.io/mt/86334815/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
