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


Zhiguang Liu
 

Hi Mike,
This code change is to avoid expose the SMM data and using CopyMem() to copy the whole structure will Copy the "next" filed which contain SMM address.
But the Guid is not private information and I think it is ok to use CopyMem() to copy Guid.
Maybe the title is confusing, I will change the patch title.

Thanks
Zhiguang

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@...>
Sent: Wednesday, June 17, 2020 6:38 AM
To: devel@edk2.groups.io; Liu, Zhiguang <zhiguang.liu@...>; Kinney,
Michael D <michael.d.kinney@...>
Cc: Zeng, Star <star.zeng@...>; Gao, Liming <liming.gao@...>;
Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>
Subject: RE: [edk2-devel] [PATCH 1/5] MdeModulePkg: avoid SMM pointers
being leaked by not using CopyMem()

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@...>; Gao, Liming
<liming.gao@...>; Wang, Jian J
<jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>
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@...>
Cc: Liming Gao <liming.gao@...>
Cc: Jian J Wang <jian.j.wang@...>
Signed-off-by: Hao A Wu <hao.a.wu@...>
Signed-off-by: Zhiguang Liu <zhiguang.liu@...>
---

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@...]
-=-=-=-=-=-=

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