在 12/23/21 7:05 PM, Marvin Häuser 写道:
> On 23.12.21 11:46, Ming Huang wrote:
>>
>> 在 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)
>
> Okay, assume:
> CommBufferEnd = MAX_UINTN
> CommBufferAddr = MAX_UINTN - 1 (MAX_UINTN - 1 < MAX_UINTN, so the check above
> would pass)
> sizeof (EFI_MM_COMMUNICATE_HEADER) = 16 (should be accurate for 64-bit
> architectures)
>
> Then (assume implicit mod 2^N on both sides due to bounded integers!):
> (CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd) <=>
> ((MAX_UINTN - 1) + 16) >= MAX_UINTN <=> MAX_UINTN + 15 >= MAX_UINTN
> <=>(wraparound!!) 14 >= MAX_UINTN <=> FALSE
>
> And (assume implicit mod 2^N on both sides due to bounded integers!):
> CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER) <=>
> MAX_UINTN - (MAX_UINTN - 1) < 16 <=> 1 < 16 <=> 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 can 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 (directly 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 (
IN UINTN BufferAddr
)
{
UINTN BufferSize;
UINT64 NsCommBufferEnd;
UINT64 SCommBufferEnd;
UINT64 CommBufferEnd;
NsCommBufferEnd = mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize;
SCommBufferEnd = mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize;
if ((BufferAddr >= mNsCommBuffer.PhysicalStart) &&
(BufferAddr < NsCommBufferEnd)) {
CommBufferEnd = NsCommBufferEnd;
} else if ((BufferAddr >= mSCommBuffer.PhysicalStart) &&
(BufferAddr < SCommBufferEnd)) {
CommBufferEnd = SCommBufferEnd;
} else {
return EFI_ACCESS_DENIED;
}
if ((CommBufferEnd - BufferAddr) < sizeof (EFI_MM_COMMUNICATE_HEADER)) {
return EFI_INVALID_PARAMETER;
}
// Find out the size of the buffer passed
BufferSize = ((EFI_MM_COMMUNICATE_HEADER *) BufferAddr)->MessageLength +
sizeof (EFI_MM_COMMUNICATE_HEADER);
// perform bounds check.
if ((CommBufferEnd - BufferAddr) < BufferSize) {
return EFI_ACCESS_DENIED;
}
return EFI_SUCCESS;
}
----------------------------------------
- Ming
>
>>> 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.
>
> 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
>>> 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 (#85206): https://edk2.groups.io/g/devel/message/85206
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]
-=-=-=-=-=-=-=-=-=-=-=-
