* [PATCH 1/1] FatPkg/EnhancedFatDxe Fix Double Cluster Allocation
@ 2018-09-07 0:07 Robinson, Herbie
2018-09-11 16:02 ` Laszlo Ersek
2018-09-13 2:45 ` Ni, Ruiyu
0 siblings, 2 replies; 4+ messages in thread
From: Robinson, Herbie @ 2018-09-07 0:07 UTC (permalink / raw)
To: edk2-devel@lists.01.org
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>
---
diff --git a/FatPkg/EnhancedFatDxe/FileSpace.c b/FatPkg/EnhancedFatDxe/FileSpace.c
index 1254cd6..e17d3b6 100644
--- a/FatPkg/EnhancedFatDxe/FileSpace.c
+++ b/FatPkg/EnhancedFatDxe/FileSpace.c
@@ -467,7 +467,7 @@ FatGrowEof (
ClusterCount = 0;
while (!FAT_END_OF_FAT_CHAIN (Cluster)) {
- if (Cluster == FAT_CLUSTER_FREE || Cluster >= FAT_CLUSTER_SPECIAL) {
+ if (Cluster < FAT_MIN_CLUSTER || Cluster > Volume->MaxCluster + 1) {
DEBUG (
(EFI_D_INIT | EFI_D_ERROR,
@@ -509,6 +509,11 @@ FatGrowEof (
goto Done;
}
+ if (NewCluster < FAT_MIN_CLUSTER || NewCluster > Volume->MaxCluster + 1) {
+ Status = EFI_VOLUME_CORRUPTED;
+ goto Done;
+ }
+
if (LastCluster != 0) {
FatSetFatEntry (Volume, LastCluster, NewCluster);
} else {
@@ -518,12 +523,21 @@ FatGrowEof (
LastCluster = NewCluster;
CurSize += 1;
+
+ //
+ // Terminate the cluster list
+ //
+ // Note that we must do this EVERY time we allocate a cluster, because
+ // FatAllocateCluster scans the FAT looking for a free cluster and
+ // "LastCluster" is no longer free! Usually, FatAllocateCluster will
+ // start looking with the cluster after "LastCluster"; however, when
+ // there is only one free cluster left, it will find "LastCluster"
+ // a second time. There are other, less predictable scenarios
+ // where this could happen, as well.
+ //
+ FatSetFatEntry (Volume, LastCluster, (UINTN) FAT_CLUSTER_LAST);
+ OFile->FileLastCluster = LastCluster;
}
- //
- // Terminate the cluster list
- //
- FatSetFatEntry (Volume, LastCluster, (UINTN) FAT_CLUSTER_LAST);
- OFile->FileLastCluster = LastCluster;
}
OFile->FileSize = (UINTN) NewSizeInBytes;
@@ -603,7 +617,7 @@ FatOFilePosition (
Cluster = FatGetFatEntry (Volume, Cluster);
}
- if (Cluster < FAT_MIN_CLUSTER) {
+ if (Cluster < FAT_MIN_CLUSTER || Cluster > Volume->MaxCluster + 1) {
return EFI_VOLUME_CORRUPTED;
}
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] FatPkg/EnhancedFatDxe Fix Double Cluster Allocation
@ 2018-09-07 3:22 Robinson, Herbie
0 siblings, 0 replies; 4+ messages in thread
From: Robinson, Herbie @ 2018-09-07 3:22 UTC (permalink / raw)
To: edk2-devel@lists.01.org
I forgot to mention this is entered as Bug 1157 in Bugzilla.
Herbie Robinson
Software Architect
Stratus Technologies | www.stratus.com
5 Mill and Main Place, Suite 500 | Maynard, MA 01754
T: +1-978-461-7531 | E: Herbie.Robinson@stratus.com
[Stratus Technologies]<http://go.stratus.com/US>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] FatPkg/EnhancedFatDxe Fix Double Cluster Allocation
2018-09-07 0:07 [PATCH 1/1] FatPkg/EnhancedFatDxe Fix Double Cluster Allocation Robinson, Herbie
@ 2018-09-11 16:02 ` Laszlo Ersek
2018-09-13 2:45 ` Ni, Ruiyu
1 sibling, 0 replies; 4+ messages in thread
From: Laszlo Ersek @ 2018-09-11 16:02 UTC (permalink / raw)
To: Robinson, Herbie; +Cc: edk2-devel@lists.01.org, Ruiyu Ni
On 09/07/18 02:07, Robinson, Herbie wrote:
> 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>
CC'ing Ray (see "Maintainers.txt").
Thanks
Laszlo
> ---
> diff --git a/FatPkg/EnhancedFatDxe/FileSpace.c b/FatPkg/EnhancedFatDxe/FileSpace.c
> index 1254cd6..e17d3b6 100644
> --- a/FatPkg/EnhancedFatDxe/FileSpace.c
> +++ b/FatPkg/EnhancedFatDxe/FileSpace.c
> @@ -467,7 +467,7 @@ FatGrowEof (
> ClusterCount = 0;
>
> while (!FAT_END_OF_FAT_CHAIN (Cluster)) {
> - if (Cluster == FAT_CLUSTER_FREE || Cluster >= FAT_CLUSTER_SPECIAL) {
> + if (Cluster < FAT_MIN_CLUSTER || Cluster > Volume->MaxCluster + 1) {
>
> DEBUG (
> (EFI_D_INIT | EFI_D_ERROR,
> @@ -509,6 +509,11 @@ FatGrowEof (
> goto Done;
> }
>
> + if (NewCluster < FAT_MIN_CLUSTER || NewCluster > Volume->MaxCluster + 1) {
> + Status = EFI_VOLUME_CORRUPTED;
> + goto Done;
> + }
> +
> if (LastCluster != 0) {
> FatSetFatEntry (Volume, LastCluster, NewCluster);
> } else {
> @@ -518,12 +523,21 @@ FatGrowEof (
>
> LastCluster = NewCluster;
> CurSize += 1;
> +
> + //
> + // Terminate the cluster list
> + //
> + // Note that we must do this EVERY time we allocate a cluster, because
> + // FatAllocateCluster scans the FAT looking for a free cluster and
> + // "LastCluster" is no longer free! Usually, FatAllocateCluster will
> + // start looking with the cluster after "LastCluster"; however, when
> + // there is only one free cluster left, it will find "LastCluster"
> + // a second time. There are other, less predictable scenarios
> + // where this could happen, as well.
> + //
> + FatSetFatEntry (Volume, LastCluster, (UINTN) FAT_CLUSTER_LAST);
> + OFile->FileLastCluster = LastCluster;
> }
> - //
> - // Terminate the cluster list
> - //
> - FatSetFatEntry (Volume, LastCluster, (UINTN) FAT_CLUSTER_LAST);
> - OFile->FileLastCluster = LastCluster;
> }
>
> OFile->FileSize = (UINTN) NewSizeInBytes;
> @@ -603,7 +617,7 @@ FatOFilePosition (
> Cluster = FatGetFatEntry (Volume, Cluster);
> }
>
> - if (Cluster < FAT_MIN_CLUSTER) {
> + if (Cluster < FAT_MIN_CLUSTER || Cluster > Volume->MaxCluster + 1) {
> return EFI_VOLUME_CORRUPTED;
> }
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] FatPkg/EnhancedFatDxe Fix Double Cluster Allocation
2018-09-07 0:07 [PATCH 1/1] FatPkg/EnhancedFatDxe Fix Double Cluster Allocation Robinson, Herbie
2018-09-11 16:02 ` Laszlo Ersek
@ 2018-09-13 2:45 ` Ni, Ruiyu
1 sibling, 0 replies; 4+ messages in thread
From: Ni, Ruiyu @ 2018-09-13 2:45 UTC (permalink / raw)
To: Robinson, Herbie, edk2-devel@lists.01.org
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
Thanks/Ray
> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of
> Robinson, Herbie
> Sent: Friday, September 7, 2018 8:07 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH 1/1] 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>
> ---
> diff --git a/FatPkg/EnhancedFatDxe/FileSpace.c
> b/FatPkg/EnhancedFatDxe/FileSpace.c
> index 1254cd6..e17d3b6 100644
> --- a/FatPkg/EnhancedFatDxe/FileSpace.c
> +++ b/FatPkg/EnhancedFatDxe/FileSpace.c
> @@ -467,7 +467,7 @@ FatGrowEof (
> ClusterCount = 0;
>
> while (!FAT_END_OF_FAT_CHAIN (Cluster)) {
> - if (Cluster == FAT_CLUSTER_FREE || Cluster >= FAT_CLUSTER_SPECIAL) {
> + if (Cluster < FAT_MIN_CLUSTER || Cluster > Volume->MaxCluster + 1) {
>
> DEBUG (
> (EFI_D_INIT | EFI_D_ERROR,
> @@ -509,6 +509,11 @@ FatGrowEof (
> goto Done;
> }
>
> + if (NewCluster < FAT_MIN_CLUSTER || NewCluster > Volume-
> >MaxCluster + 1) {
> + Status = EFI_VOLUME_CORRUPTED;
> + goto Done;
> + }
> +
> if (LastCluster != 0) {
> FatSetFatEntry (Volume, LastCluster, NewCluster);
> } else {
> @@ -518,12 +523,21 @@ FatGrowEof (
>
> LastCluster = NewCluster;
> CurSize += 1;
> +
> + //
> + // Terminate the cluster list
> + //
> + // Note that we must do this EVERY time we allocate a cluster, because
> + // FatAllocateCluster scans the FAT looking for a free cluster and
> + // "LastCluster" is no longer free! Usually, FatAllocateCluster will
> + // start looking with the cluster after "LastCluster"; however, when
> + // there is only one free cluster left, it will find "LastCluster"
> + // a second time. There are other, less predictable scenarios
> + // where this could happen, as well.
> + //
> + FatSetFatEntry (Volume, LastCluster, (UINTN) FAT_CLUSTER_LAST);
> + OFile->FileLastCluster = LastCluster;
> }
> - //
> - // Terminate the cluster list
> - //
> - FatSetFatEntry (Volume, LastCluster, (UINTN) FAT_CLUSTER_LAST);
> - OFile->FileLastCluster = LastCluster;
> }
>
> OFile->FileSize = (UINTN) NewSizeInBytes;
> @@ -603,7 +617,7 @@ FatOFilePosition (
> Cluster = FatGetFatEntry (Volume, Cluster);
> }
>
> - if (Cluster < FAT_MIN_CLUSTER) {
> + if (Cluster < FAT_MIN_CLUSTER || Cluster > Volume->MaxCluster + 1) {
> return EFI_VOLUME_CORRUPTED;
> }
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-09-13 2:48 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-09-07 0:07 [PATCH 1/1] FatPkg/EnhancedFatDxe Fix Double Cluster Allocation Robinson, Herbie
2018-09-11 16:02 ` Laszlo Ersek
2018-09-13 2:45 ` Ni, Ruiyu
-- strict thread matches above, loose matches on Subject: below --
2018-09-07 3:22 Robinson, Herbie
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox