FatPkg/EnhancedFatDxe Fix Double Cluster Allocation

This is a fix for a double cluster allocation when the disk is full.
The double allocation happens because FatGrowEof calls
FatAllocateCluster without immediately marking the each returned
cluster as allocated. The fix is to move the FatSetFatEntry call
inside the loop.
I've also include some improvements to the sanity checks that I added
while tracking this down. They are optional.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Herbie Robinson <Herbie.Robinson@stratus.com>
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

diff --git a/FatPkg/EnhancedFatDxe/FileSpace.c b/FatPkg/EnhancedFatDxe/FileSpace.c
index 1254cd68b55a643f760889e3377944a2db9a1e64..50391b75b88453999f3a1659dd128fc0e7c5d102 100644 (file)
--- a/FatPkg/EnhancedFatDxe/FileSpace.c
+++ b/FatPkg/EnhancedFatDxe/FileSpace.c
@@ -467,7 +467,7 @@ FatGrowEof (
       ClusterCount  = 0;\r
 \r
       while (!FAT_END_OF_FAT_CHAIN (Cluster)) {\r
-        if (Cluster == FAT_CLUSTER_FREE || Cluster >= FAT_CLUSTER_SPECIAL) {\r
+        if (Cluster < FAT_MIN_CLUSTER || Cluster > Volume->MaxCluster + 1) {\r
 \r
           DEBUG (\r
             (EFI_D_INIT | EFI_D_ERROR,\r
@@ -509,6 +509,11 @@ FatGrowEof (
         goto Done;\r
       }\r
 \r
+      if (NewCluster < FAT_MIN_CLUSTER || NewCluster > Volume->MaxCluster + 1) {
+        Status = EFI_VOLUME_CORRUPTED;
+        goto Done;
+      }
+
       if (LastCluster != 0) {\r
         FatSetFatEntry (Volume, LastCluster, NewCluster);\r
       } else {\r
@@ -518,12 +523,21 @@ FatGrowEof (
 \r
       LastCluster = NewCluster;\r
       CurSize += 1;\r
+\r
+      //\r
+      // Terminate the cluster list\r
+      //\r
+      // Note that we must do this EVERY time we allocate a cluster, because\r
+      // FatAllocateCluster scans the FAT looking for a free cluster and\r
+      // "LastCluster" is no longer free!  Usually, FatAllocateCluster will\r
+      // start looking with the cluster after "LastCluster"; however, when\r
+      // there is only one free cluster left, it will find "LastCluster"\r
+      // a second time.  There are other, less predictable scenarios\r
+      // where this could happen, as well.\r
+      //\r
+      FatSetFatEntry (Volume, LastCluster, (UINTN) FAT_CLUSTER_LAST);\r
+      OFile->FileLastCluster = LastCluster;\r
     }\r
-    //\r
-    // Terminate the cluster list\r
-    //\r
-    FatSetFatEntry (Volume, LastCluster, (UINTN) FAT_CLUSTER_LAST);\r
-    OFile->FileLastCluster = LastCluster;\r
   }\r
 \r
   OFile->FileSize = (UINTN) NewSizeInBytes;\r
@@ -603,7 +617,7 @@ FatOFilePosition (
       Cluster = FatGetFatEntry (Volume, Cluster);\r
     }\r
 \r
-    if (Cluster < FAT_MIN_CLUSTER) {\r
+    if (Cluster < FAT_MIN_CLUSTER || Cluster > Volume->MaxCluster + 1) {\r
       return EFI_VOLUME_CORRUPTED;\r
     }\r
 \r