On 24.12.21 02:18, Ming Huang wrote:
在 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;
Just another cosmetic thing I just noticed, why is this Invalid
Parameter? The check above yields Access Denied when the buffer start is
out of a trusted 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 policy of function documentation (i.e. whether
one is needed), but what would the difference be in their descriptions?
}
// Find out the size of the buffer passed
BufferSize = ((EFI_MM_COMMUNICATE_HEADER *) BufferAddr)->MessageLength +
sizeof (EFI_MM_COMMUNICATE_HEADER);
Same issue as above, can also be solved by rewriting as subtraction.
Because you now know that (CommBufferEnd - BufferAddr) >= sizeof
(EFI_MM_COMMUNICATE_HEADER).
Best regards,
Marvin
// 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 (#85212): https://edk2.groups.io/g/devel/message/85212
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]
-=-=-=-=-=-=-=-=-=-=-=-
