Re: [PATCH] MdeModulePkg/SmmCore: Fix hang due to already-freed memory deference

["Shi, Steven" <steven.shi@intel.com>]

Hi Laszlo,
Check the DispatchHandle valid in internal handle set before using it to reference its Signature data is majorly to avoid use-after-free problem here, it also can defense if an input handle is invalid but has a valid signature occasionally or deliberately. 

> Generally, if client code violates an interface contract, then the called function is not responsible for catching the error and preventing undefined behavior. 
> For "quality of service", we can go to certain lengths nonetheless, but it should hopefully not hurt valid client code.
If the called function is an interface function, I think it is necessary to validate the inputs before use them to reference other internal data. This way can make the service code more secure.


Steven Shi
Intel\SSG\STO\UEFI Firmware

Tel: +86 021-61166522
iNet: 821-6522

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Friday, February 2, 2018 12:12 AM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH] MdeModulePkg/SmmCore: Fix hang due to
> already-freed memory deference
> 
> Hello Ray,
> 
> On 02/01/18 11:15, Ruiyu Ni wrote:
> > SmiHandlerUnRegister() validates the DispatchHandle by checking
> > whether the first 32bit matches to a certain signature
> > (SMI_HANDLER_SIGNATURE).
> > But if a caller calls *UnRegister() twice and the memory freed by
> > first call still contains the signature, the second hang may hang.
> >
> > The patch fixes this issue by locating the DispatchHandle
> > in all SMI handlers, instead of checking the signature.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > ---
> >  MdeModulePkg/Core/PiSmmCore/Smi.c | 37
> ++++++++++++++++++++++++++++++++-----
> >  1 file changed, 32 insertions(+), 5 deletions(-)
> 
> I'm mildly curious: can we just zero out the signature when the
> de-registration / freeing happens? Otherwise, the nested loop added
> below will penalize (performance-wise) correctly written client code as
> well.
> 
> > diff --git a/MdeModulePkg/Core/PiSmmCore/Smi.c
> > b/MdeModulePkg/Core/PiSmmCore/Smi.c
> > index ad483a1877ce..6596ea9560d1 100644
> > --- a/MdeModulePkg/Core/PiSmmCore/Smi.c
> > +++ b/MdeModulePkg/Core/PiSmmCore/Smi.c
> > @@ -290,6 +290,7 @@ SmiHandlerUnRegister (
> >    SmiEntry = SmiHandler->SmiEntry;
> >
> >    RemoveEntryList (&SmiHandler->Link);
> > +  SmiHandler->Signature = 0;
> >    FreePool (SmiHandler);
> >
> >    if (SmiEntry == NULL) {
> 
> Generally, if client code violates an interface contract, then the
> called function is not responsible for catching the error and preventing
> undefined behavior. For "quality of service", we can go to certain
> lengths nonetheless, but it should hopefully not hurt valid client code.
> 
> For example, I seem to remember that the list data structure
> implementation checks the internal consistency (which can be costly)
> only if a PCD is set to a certain value. Is that right? Is it an option
> here? (If the above zeroing is not good for some reason.)
> 
> Anyway, I'm asking mainly for my own education.
> 
> Thanks!
> Laszlo
> 
> > diff --git a/MdeModulePkg/Core/PiSmmCore/Smi.c
> b/MdeModulePkg/Core/PiSmmCore/Smi.c
> > index ad483a1877..0c09e7fa10 100644
> > --- a/MdeModulePkg/Core/PiSmmCore/Smi.c
> > +++ b/MdeModulePkg/Core/PiSmmCore/Smi.c
> > @@ -1,7 +1,7 @@
> >  /** @file
> >    SMI management.
> >
> > -  Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.<BR>
> > +  Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
> >    This program and the accompanying materials are licensed and made
> available
> >    under the terms and conditions of the BSD License which accompanies
> this
> >    distribution.  The full text of the license may be found at
> > @@ -276,14 +276,41 @@ SmiHandlerUnRegister (
> >  {
> >    SMI_HANDLER  *SmiHandler;
> >    SMI_ENTRY    *SmiEntry;
> > +  LIST_ENTRY   *EntryLink;
> > +  LIST_ENTRY   *HandlerLink;
> >
> > -  SmiHandler = (SMI_HANDLER *) DispatchHandle;
> > -
> > -  if (SmiHandler == NULL) {
> > +  if (DispatchHandle == NULL) {
> >      return EFI_INVALID_PARAMETER;
> >    }
> >
> > -  if (SmiHandler->Signature != SMI_HANDLER_SIGNATURE) {
> > +  //
> > +  // Look for it in root SMI handlers
> > +  //
> > +  SmiHandler = NULL;
> > +  for ( HandlerLink = GetFirstNode (&mRootSmiEntry.SmiHandlers)
> > +      ; !IsNull (&mRootSmiEntry.SmiHandlers, HandlerLink) &&
> (SmiHandler != DispatchHandle)
> > +      ; HandlerLink = GetNextNode (&mRootSmiEntry.SmiHandlers,
> HandlerLink)
> > +      ) {
> > +    SmiHandler = CR (HandlerLink, SMI_HANDLER, Link,
> SMI_HANDLER_SIGNATURE);
> > +  }
> > +
> > +  //
> > +  // Look for it in non-root SMI handlers
> > +  //
> > +  for ( EntryLink = GetFirstNode (&mSmiEntryList)
> > +      ; !IsNull (&mSmiEntryList, EntryLink) && (SmiHandler !=
> DispatchHandle)
> > +      ; EntryLink = GetNextNode (&mSmiEntryList, EntryLink)
> > +      ) {
> > +    SmiEntry = CR (EntryLink, SMI_ENTRY, AllEntries,
> SMI_ENTRY_SIGNATURE);
> > +    for ( HandlerLink = GetFirstNode (&SmiEntry->SmiHandlers)
> > +        ; !IsNull (&SmiEntry->SmiHandlers, HandlerLink) && (SmiHandler !=
> DispatchHandle)
> > +        ; HandlerLink = GetNextNode (&SmiEntry->SmiHandlers, HandlerLink)
> > +        ) {
> > +      SmiHandler = CR (HandlerLink, SMI_HANDLER, Link,
> SMI_HANDLER_SIGNATURE);
> > +    }
> > +  }
> > +
> > +  if (SmiHandler != DispatchHandle) {
> >      return EFI_INVALID_PARAMETER;
> >    }
> >
> >
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel