Re: [PATCH 1/5] MdeModulePkg: avoid SMM pointers being leaked by not using CopyMem()


Michael D Kinney
 

Zhiguang,

An implementation of CopyGuid() could use CopyMem().
Does CopyGuid() also need to be avoided?

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On
Behalf Of Zhiguang Liu
Sent: Tuesday, June 16, 2020 2:05 AM
To: devel@edk2.groups.io
Cc: Zeng, Star <star.zeng@intel.com>; Gao, Liming
<liming.gao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
Subject: [edk2-devel] [PATCH 1/5] MdeModulePkg: avoid
SMM pointers being leaked by not using CopyMem()

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2002

This commit will update the logic in function
SmmVariableGetStatistics()
so that the pointer fields ('Next' and 'Name') in
structure
VARIABLE_INFO_ENTRY will not be copied into the SMM
communication buffer.

Doing so will prevent SMM pointers address from being
leaked into non-SMM
environment.

Please note that newly introduced internal function
CopyVarInfoEntry()
will not use CopyMem() to copy the whole
VARIABLE_INFO_ENTRY structure and
then zero out the 'Next' and 'Name' fields. This is for
preventing race
conditions where the pointers value might still be read.

Cc: Star Zeng <star.zeng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---

MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
| 33 +++++++++++++++++++++++++++++++--
1 file changed, 31 insertions(+), 2 deletions(-)

diff --git
a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm
.c
b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm
.c
index caca5c3241..74e756bc00 100644
---
a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm
.c
+++
b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm
.c
@@ -315,6 +315,35 @@ GetFvbCountAndBuffer (
}





+/**

+ Copy only the meaningful fields of the variable
statistics information from

+ source buffer to the destination buffer. Other fields
are filled with zero.

+

+ @param[out] DstInfoEntry A pointer to the buffer
of destination variable

+ information entry.

+ @param[in] SrcInfoEntry A pointer to the buffer
of source variable

+ information entry.

+

+**/

+static

+VOID

+CopyVarInfoEntry (

+ OUT VARIABLE_INFO_ENTRY *DstInfoEntry,

+ IN VARIABLE_INFO_ENTRY *SrcInfoEntry

+ )

+{

+ DstInfoEntry->Next = NULL;

+ DstInfoEntry->Name = NULL;

+

+ CopyGuid (&DstInfoEntry->VendorGuid, &SrcInfoEntry-
VendorGuid);
+ DstInfoEntry->Attributes = SrcInfoEntry->Attributes;

+ DstInfoEntry->ReadCount = SrcInfoEntry->ReadCount;

+ DstInfoEntry->WriteCount = SrcInfoEntry->WriteCount;

+ DstInfoEntry->DeleteCount = SrcInfoEntry-
DeleteCount;
+ DstInfoEntry->CacheCount = SrcInfoEntry->CacheCount;

+ DstInfoEntry->Volatile = SrcInfoEntry->Volatile;

+}

+

/**

Get the variable statistics information from the
information buffer pointed by gVariableInfo.



@@ -377,7 +406,7 @@ SmmVariableGetStatistics (
*InfoSize = StatisticsInfoSize;

return EFI_BUFFER_TOO_SMALL;

}

- CopyMem (InfoEntry, VariableInfo, sizeof
(VARIABLE_INFO_ENTRY));

+ CopyVarInfoEntry (InfoEntry, VariableInfo);

CopyMem (InfoName, VariableInfo->Name, NameSize);

*InfoSize = StatisticsInfoSize;

return EFI_SUCCESS;

@@ -417,7 +446,7 @@ SmmVariableGetStatistics (
return EFI_BUFFER_TOO_SMALL;

}



- CopyMem (InfoEntry, VariableInfo, sizeof
(VARIABLE_INFO_ENTRY));

+ CopyVarInfoEntry (InfoEntry, VariableInfo);

CopyMem (InfoName, VariableInfo->Name, NameSize);

*InfoSize = StatisticsInfoSize;



--
2.25.1.windows.1


-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this
group.

View/Reply Online (#61324):
https://edk2.groups.io/g/devel/message/61324
Mute This Topic: https://groups.io/mt/74912557/1643496
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub
[michael.d.kinney@intel.com]
-=-=-=-=-=-=

Join devel@edk2.groups.io to automatically receive all group messages.