[PATCH] FatPkg/EnhancedFatDxe: Fix potential hang in async file IO

[PATCH] FatPkg/EnhancedFatDxe: Fix potential hang in async file IO

[Ruiyu Ni]

FatQueueTask() is running at TPL_APPLICATION, while
FatDestroySubtask() is running at TPL_NOTIFY, it's possible
for a task containing 2 sub tasks, when the for-loop
executes GetNextNode (&Task->Subtasks, Link), the memory
occupied by Link is freed in FatDestroySubtask().

The fix stores the next link in NextLink so that the delete
in FatDestroySubtask() is safe.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Feng Tian <feng.tian@intel.com>
Cc: Boaz Kahana <Boaz.Kahana@phoenix.com>
---
 FatPkg/EnhancedFatDxe/Misc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/FatPkg/EnhancedFatDxe/Misc.c b/FatPkg/EnhancedFatDxe/Misc.c
index c035670..cef1acd 100644
--- a/FatPkg/EnhancedFatDxe/Misc.c
+++ b/FatPkg/EnhancedFatDxe/Misc.c
@@ -1,7 +1,7 @@
 /** @file
   Miscellaneous functions.
 
-Copyright (c) 2005 - 2013, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2005 - 2016, 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
@@ -132,6 +132,7 @@ FatQueueTask (
 {
   EFI_STATUS          Status;
   LIST_ENTRY          *Link;
+  LIST_ENTRY          *NextLink;
   FAT_SUBTASK         *Subtask;
 
   //
@@ -149,9 +150,13 @@ FatQueueTask (
   EfiReleaseLock (&FatTaskLock);
 
   Status = EFI_SUCCESS;
-  for ( Link = GetFirstNode (&Task->Subtasks)
+  //
+  // Use NextLink to store the next link since Link might be freed in the end of previous loop,
+  // resulting next link cannot be retrieved from Link.
+  //
+  for ( Link = GetFirstNode (&Task->Subtasks), NextLink = GetNextNode (&Task->Subtasks, Link)
       ; !IsNull (&Task->Subtasks, Link)
-      ; Link = GetNextNode (&Task->Subtasks, Link)
+      ; Link = NextLink, NextLink = GetNextNode (&Task->Subtasks, Link)
       ) {
     Subtask = CR (Link, FAT_SUBTASK, Link, FAT_SUBTASK_SIGNATURE);
     if (Subtask->Write) {
-- 
2.9.0.windows.1

Re: [PATCH] FatPkg/EnhancedFatDxe: Fix potential hang in async file IO

[Ni, Ruiyu]

Boaz,
Could you please verify this patch in your failed system?
I cannot reproduce the issue in my environment.

Thanks/Ray

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Ruiyu Ni
> Sent: Wednesday, December 14, 2016 6:01 PM
> To: edk2-devel@lists.01.org
> Cc: Tian, Feng <feng.tian@intel.com>; Boaz Kahana
> <Boaz.Kahana@phoenix.com>
> Subject: [edk2] [PATCH] FatPkg/EnhancedFatDxe: Fix potential hang in async
> file IO
> 
> FatQueueTask() is running at TPL_APPLICATION, while
> FatDestroySubtask() is running at TPL_NOTIFY, it's possible for a task
> containing 2 sub tasks, when the for-loop executes GetNextNode (&Task-
> >Subtasks, Link), the memory occupied by Link is freed in
> FatDestroySubtask().
> 
> The fix stores the next link in NextLink so that the delete in
> FatDestroySubtask() is safe.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Boaz Kahana <Boaz.Kahana@phoenix.com>
> ---
>  FatPkg/EnhancedFatDxe/Misc.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/FatPkg/EnhancedFatDxe/Misc.c b/FatPkg/EnhancedFatDxe/Misc.c
> index c035670..cef1acd 100644
> --- a/FatPkg/EnhancedFatDxe/Misc.c
> +++ b/FatPkg/EnhancedFatDxe/Misc.c
> @@ -1,7 +1,7 @@
>  /** @file
>    Miscellaneous functions.
> 
> -Copyright (c) 2005 - 2013, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2005 - 2016, 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
> @@ -132,6 +132,7 @@ FatQueueTask (  {
>    EFI_STATUS          Status;
>    LIST_ENTRY          *Link;
> +  LIST_ENTRY          *NextLink;
>    FAT_SUBTASK         *Subtask;
> 
>    //
> @@ -149,9 +150,13 @@ FatQueueTask (
>    EfiReleaseLock (&FatTaskLock);
> 
>    Status = EFI_SUCCESS;
> -  for ( Link = GetFirstNode (&Task->Subtasks)
> +  //
> +  // Use NextLink to store the next link since Link might be freed in
> + the end of previous loop,  // resulting next link cannot be retrieved from
> Link.
> +  //
> +  for ( Link = GetFirstNode (&Task->Subtasks), NextLink = GetNextNode
> + (&Task->Subtasks, Link)
>        ; !IsNull (&Task->Subtasks, Link)
> -      ; Link = GetNextNode (&Task->Subtasks, Link)
> +      ; Link = NextLink, NextLink = GetNextNode (&Task->Subtasks, Link)
>        ) {
>      Subtask = CR (Link, FAT_SUBTASK, Link, FAT_SUBTASK_SIGNATURE);
>      if (Subtask->Write) {
> --
> 2.9.0.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel