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

["Zhiguang Liu" <zhiguang.liu@intel.com>]

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@intel.com>
> Sent: Wednesday, June 17, 2020 6:38 AM
> To: devel@edk2.groups.io; Liu, Zhiguang <zhiguang.liu@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>
> 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: 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@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]
> > -=-=-=-=-=-=