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:[email protected]] On Behalf Of > Laszlo Ersek > Sent: Friday, February 2, 2018 12:12 AM > To: Ni, Ruiyu <[email protected]>; [email protected] > Cc: Yao, Jiewen <[email protected]>; Zeng, Star <[email protected]> > 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 <[email protected]> > > Cc: Jiewen Yao <[email protected]> > > Cc: Star Zeng <[email protected]> > > --- > > 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 > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
