[PATCH v2] MdeModulePkg/Variable/RuntimeDxe: Not copy SMM pointers in comm buffer

[PATCH v2] MdeModulePkg/Variable/RuntimeDxe: Not copy SMM pointers in comm buffer

[Wu, Hao A]

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>
---

V2 patch updates the type of parameter 'DstInfoEntry' from 'in' to 'out'
for the newly added function CopyVarInfoEntry().


 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 ec463d063e..9b59387e58 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
@@ -312,6 +312,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.
 
   Caution: This function may be invoked at SMM runtime.
@@ -373,7 +402,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;
@@ -413,7 +442,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.12.0.windows.1

Re: [edk2-devel] [PATCH v2] MdeModulePkg/Variable/RuntimeDxe: Not copy SMM pointers in comm buffer

[Wang, Jian J]

This is still a security bug fix, which needs CVE number added in title. With it
addressed,

Reviewed-by: Jian J Wang <jian.j.wang@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Wu, Hao A
> Sent: Wednesday, August 07, 2019 8:18 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Zeng, Star <star.zeng@intel.com>;
> Gao, Liming <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
> Subject: [edk2-devel] [PATCH v2] MdeModulePkg/Variable/RuntimeDxe:
> Not copy SMM pointers in comm buffer
> 
> 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>
> ---
> 
> V2 patch updates the type of parameter 'DstInfoEntry' from 'in' to 'out'
> for the newly added function CopyVarInfoEntry().
> 
> 
>  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 ec463d063e..9b59387e58 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> @@ -312,6 +312,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.
> 
>    Caution: This function may be invoked at SMM runtime.
> @@ -373,7 +402,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;
> @@ -413,7 +442,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.12.0.windows.1
> 
> 
> 

Re: [edk2-devel] [PATCH v2] MdeModulePkg/Variable/RuntimeDxe: Not copy SMM pointers in comm buffer

[Yao, Jiewen]

If this is considered as a security bug, we need fix all possible information leak. Not only this one.
For example, the SMM data is installed the DXE database, such as loaded image protocol.
Just fixing in one place is not good enough to resolve all concern.
To me, this patch is like to harden the door, but leave windows open.
It is necessary but not sufficient.

Thank you
Yao Jiewen


> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Wang, Jian J
> Sent: Wednesday, August 7, 2019 11:22 AM
> To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>
> Cc: Zeng, Star <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/Variable/RuntimeDxe:
> Not copy SMM pointers in comm buffer
> 
> This is still a security bug fix, which needs CVE number added in title. With it
> addressed,
> 
> Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> > Wu, Hao A
> > Sent: Wednesday, August 07, 2019 8:18 AM
> > To: devel@edk2.groups.io
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Zeng, Star <star.zeng@intel.com>;
> > Gao, Liming <liming.gao@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>
> > Subject: [edk2-devel] [PATCH v2] MdeModulePkg/Variable/RuntimeDxe:
> > Not copy SMM pointers in comm buffer
> >
> > 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>
> > ---
> >
> > V2 patch updates the type of parameter 'DstInfoEntry' from 'in' to 'out'
> > for the newly added function CopyVarInfoEntry().
> >
> >
> >  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 ec463d063e..9b59387e58 100644
> > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> > @@ -312,6 +312,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.
> >
> >    Caution: This function may be invoked at SMM runtime.
> > @@ -373,7 +402,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;
> > @@ -413,7 +442,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.12.0.windows.1
> >
> >
> >
> 
> 
>