Date
1 - 3 of 3
[PATCH 1/5] MdeModulePkg: avoid SMM pointers being leaked by not using CopyMem()
|
Zhiguang Liu
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2002
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/Mde= ModulePkg/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 ( }=0D =0D =0D +/**=0D + Copy only the meaningful fields of the variable statistics information f= rom=0D + source buffer to the destination buffer. Other fields are filled with ze= ro.=0D +=0D + @param[out] DstInfoEntry A pointer to the buffer of destination vari= able=0D + information entry.=0D + @param[in] SrcInfoEntry A pointer to the buffer of source variable= =0D + information entry.=0D +=0D +**/=0D +static=0D +VOID=0D +CopyVarInfoEntry (=0D + OUT VARIABLE_INFO_ENTRY *DstInfoEntry,=0D + IN VARIABLE_INFO_ENTRY *SrcInfoEntry=0D + )=0D +{=0D + DstInfoEntry->Next =3D NULL;=0D + DstInfoEntry->Name =3D NULL;=0D +=0D + CopyGuid (&DstInfoEntry->VendorGuid, &SrcInfoEntry->VendorGuid);=0D + DstInfoEntry->Attributes =3D SrcInfoEntry->Attributes;=0D + DstInfoEntry->ReadCount =3D SrcInfoEntry->ReadCount;=0D + DstInfoEntry->WriteCount =3D SrcInfoEntry->WriteCount;=0D + DstInfoEntry->DeleteCount =3D SrcInfoEntry->DeleteCount;=0D + DstInfoEntry->CacheCount =3D SrcInfoEntry->CacheCount;=0D + DstInfoEntry->Volatile =3D SrcInfoEntry->Volatile;=0D +}=0D +=0D /**=0D Get the variable statistics information from the information buffer poin= ted by gVariableInfo.=0D =0D @@ -377,7 +406,7 @@ SmmVariableGetStatistics ( *InfoSize =3D StatisticsInfoSize;=0D return EFI_BUFFER_TOO_SMALL;=0D }=0D - CopyMem (InfoEntry, VariableInfo, sizeof (VARIABLE_INFO_ENTRY));=0D + CopyVarInfoEntry (InfoEntry, VariableInfo);=0D CopyMem (InfoName, VariableInfo->Name, NameSize);=0D *InfoSize =3D StatisticsInfoSize;=0D return EFI_SUCCESS;=0D @@ -417,7 +446,7 @@ SmmVariableGetStatistics ( return EFI_BUFFER_TOO_SMALL;=0D }=0D =0D - CopyMem (InfoEntry, VariableInfo, sizeof (VARIABLE_INFO_ENTRY));=0D + CopyVarInfoEntry (InfoEntry, VariableInfo);=0D CopyMem (InfoName, VariableInfo->Name, NameSize);=0D *InfoSize =3D StatisticsInfoSize;=0D =0D --=20 2.25.1.windows.1
|
|
|
|
Michael D Kinney
Zhiguang,
toggle quoted messageShow quoted text
An implementation of CopyGuid() could use CopyMem(). Does CopyGuid() also need to be avoided? Mike
-----Original Message-----
|
|
|
|
Zhiguang Liu
Hi Mike,
toggle quoted messageShow quoted text
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-----
|
|
|