[edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

[Marvin Häuser]

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.wang at intel.com>
>>> Cc: Hao A Wu <hao.a.wu at intel.com>
>>> Cc: Eric Dong <eric.dong at intel.com>
>>> Cc: Ray Ni <ray.ni at intel.com>
>>> Cc: Michael D Kinney <michael.d.kinney at intel.com>
>>> Cc: Liming Gao <gaoliming at byosoft.com.cn>
>>> Cc: Zhiguang Liu <zhiguang.liu at intel.com>
>>> Cc: Andrew Fish <afish at apple.com>
>>> Cc: Laszlo Ersek <lersek at redhat.com>
>>> Cc: Leif Lindholm <leif at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-