Hey Kun,
On 16.06.21 22:58, Kun Qin wrote:
Hi Marvin,
Thanks for reaching out. Please see my comment inline.
Regards,
Kun
On 06/16/2021 00:02, Marvin Häuser wrote:
Good day,
May I ask about two small things?
1) Was there any rationale as to e.g. code compatibility with
choosing UINT64 for the data length? I understand that this is the
maximum of the two as of currently, but I wonder whether a message
length that exceeds the UINT32 range (4 GB!) can possibly be
considered sane or a good idea.
I agree that >4GB communication buffer may not be practical as of
today. That is why we modified the SMM communication routine in
PiSmmIpl to use SafeInt functions in Patch 2 of this series. With that
change, at least for 32bit MM, larger than 4GB will be considered
insane. But in the meantime, I do not want to rule out the >4GB
communication capability that a 64bit MM currently already have.
Please let me know if I missed anything in regards to adding SafeInt
functions to SmmCommunication routine.
I didn't see anything missed, but that is part of the point. If it was
UINT32, no such validation would be required. Also I feel like in
high-security scenarios, you would have a cap (that is well below 4 GB I
imagine) anyway past which you reject transmission for looking insane. I
totally understand it's a tough choice to reduce the capabilities and to
go with a capacity less than what is possible, but I do not think
current transmission realistically exceed a very few megabytes? This
would still allow for incredible headroom.
2) Is it feasible yet with the current set of supported compilers to
support flexible arrays?
My impression is that flexible arrays are already supported (as seen
in UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
Please correct me if I am wrong.
Would you mind letting me know why this is applicable here? We are
trying to seek ideas on how to catch developer mistakes caused by this
change. So any input is appreciated.
Huh, interesting. Last time I tried I was told about incompatibilities
with MSVC, but I know some have been dropped since then (2005 and 2008
if I recall correctly?), so that'd be great to allow globally. I feel
like if the structure is modified anyway, it should probably get a
trailing flexible array over the 1-sized hack. What do you think?
Best regards,
Marvin
Thank you for your work!
Best regards,
Marvin
On 10.06.21 03:42, Kun Qin wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3430
In PI Spec v1.7 Errata A, Vol.4, Sec 5.7 MM Communication Protocol, the
MessageLength field of EFI_MM_COMMUNICATE_HEADER (also derived as
EFI_SMM_COMMUNICATE_HEADER) is currently defined as type UINTN.
But this structure, as a generic definition, could be used for both PEI
and DXE MM communication. Thus for a system that supports PEI MM
launch,
but operates PEI in 32bit mode and MM foundation in 64bit, the current
EFI_MM_COMMUNICATE_HEADER definition will cause structure parse
error due
to UINTN being used.
The suggested change is to make the MessageLength field defined with
definitive size as below:
```
typedef struct {
EFI_GUID HeaderGuid;
UINT64 MessageLength;
UINT8 Data[ANYSIZE_ARRAY];
} EFI_MM_COMMUNICATE_HEADER;
```
Patch v1 branch:
https://github.com/kuqin12/edk2/tree/BZ3398-MmCommunicate-Length
Cc: Jian J Wang <jian.j.w...@intel.com>
Cc: Hao A Wu <hao.a...@intel.com>
Cc: Eric Dong <eric.d...@intel.com>
Cc: Ray Ni <ray...@intel.com>
Cc: Michael D Kinney <michael.d.kin...@intel.com>
Cc: Liming Gao <gaolim...@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang....@intel.com>
Cc: Andrew Fish <af...@apple.com>
Cc: Laszlo Ersek <ler...@redhat.com>
Cc: Leif Lindholm <l...@nuviainc.com>
Kun Qin (5):
EDK2 Code First: PI Specification: EFI_MM_COMMUNICATE_HEADER Update
MdeModulePkg: PiSmmIpl: Update MessageLength calculation for
MmCommunicate
MdeModulePkg: MemoryProfileInfo: Updated MessageLength calculation
MdeModulePkg: SmiHandlerProfileInfo: Updated MessageLength
calculation
MdePkg: MmCommunication: Extend MessageLength field size to UINT64
MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c | 20
+++--
MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c
| 8 +-
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 13 ++-
BZ3430-SpecChange.md | 88 ++++++++++++++++++++
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf | 1 +
MdePkg/Include/Protocol/MmCommunication.h | 3 +-
6 files changed, 124 insertions(+), 9 deletions(-)
create mode 100644 BZ3430-SpecChange.md
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76745): https://edk2.groups.io/g/devel/message/76745
Mute This Topic: https://groups.io/mt/83435921/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
