Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lack of examine the FAT table in FAT driver causes a BIOS hang when calling fs->OpenVolume() (Bugzilla Bug 1452) #9679

Open
tianocore-issues opened this issue Jan 11, 2019 · 13 comments · May be fixed by #10609
Labels
package:fatpkg priority:high Significant impact. Should be fixed as soon as possible. type:bug Something isn't working

Comments

@tianocore-issues
Copy link

This issue was created automatically with bugzilla2github

Bugzilla Bug 1452

Date: 2019-01-11T17:06:43+00:00
From: Oleksiy <>
To: unassigned <>
CC: shala.arshi, yonghong.zhu

Last updated: 2022-02-09T20:39:30+00:00

@tianocore-issues
Copy link
Author

Comment 6060

Date: 2019-01-11 17:06:43 +0000
From: Oleksiy <>

  • Industry Specification: ---
  • Release Observed: EDK II Master
  • Releases to Fix: EDK II Master
  • Target OS: ---
  • Bugzilla Assignee(s): unassigned <>

Created attachment 245
Fat Boot Sector + Fat Table

BIOS would hang when calling OpenVolume() to open a FAT32 partition formatted by newfs tool in Solaris.
The FAT driver would hang in the loop of while (!FAT_END_OF_FAT_CHAIN (Cluster)) in FatPhysicalDirSize() when it trying to get the root directory size.

See attached file for Fat Table and Fat boot sector.

This partition has a valid FAT boot sector, while its FAT table has something invalid. Since FAT driver does not examine the FAT ID and End of Chain field in the FAT table before consuming the FAT data, it eventually hang in the while loop as above.
In this case, the first chain cluster field has the value equals to root dir cluster, so it can not finish the while loop.

Code calling path: FatOpenVolume()->FatOpenDirEnt()->FatPhysicalDirSize()->FatGetFatEntry()

Proposed fix (See Addtional Checks >>):

VOID *
FatLoadFatEntry (
IN FAT_VOLUME *Volume,
IN UINTN Index
)

{
UINTN Pos;
EFI_STATUS Status;

if (Index > (Volume->MaxCluster + 1)) {
Volume->FatEntryBuffer = (UINT32) -1;
return &Volume->FatEntryBuffer;
}
//
// Compute buffer position needed
//
switch (Volume->FatType) {
case FAT12:
Pos = FAT_POS_FAT12 (Index);
break;

case FAT16:
Pos = FAT_POS_FAT16 (Index);
break;

default:
Pos = FAT_POS_FAT32 (Index);
}
//
// Set the position and read the buffer
//
Volume->FatEntryPos = Volume->FatPos + Pos;
Status = FatDiskIo (
Volume,
READ_FAT,
Volume->FatEntryPos,
Volume->FatEntrySize,
&Volume->FatEntryBuffer,
NULL
);
if (EFI_ERROR (Status)) {
Volume->FatEntryBuffer = (UINT32) -1;
}
//Addtional Checks >>
//To be safe, handle only RootCluster.
if(Index == Volume->RootCluster)
{
UINT8 Data[0x10];
UINT8 FatId = 0;
Status = FatDiskIo (
Volume,
READ_FAT,
Volume->FatEntryPos - Pos, //Read from FAT entry[0]
0x10,
Data,
NULL
);
if (!EFI_ERROR (Status)) {

  FatId = Data[0];
     
    if (FatId &lt;= 0xf7 &amp;&amp; FatId != 0xf0) //0xF8 ~ 0xFF and 0xF0 are valid values.
     Volume-&gt;FatEntryBuffer = 0;
} 

}
//Addtional Checks <<

return &Volume->FatEntryBuffer;
}

Attachment: BZ-245-1.jpg

@tianocore-issues
Copy link
Author

Comment 6137

Date: 2019-01-17 19:21:18 +0000
From: Yonghong Zhu <<yonghong.zhu>>

bug scrub: assign to module owner

@tianocore-issues
Copy link
Author

Comment 6232

Date: 2019-01-29 14:20:57 +0000
From: Oleksiy <>

Please update status.

@tianocore-issues
Copy link
Author

Comment 6240

Date: 2019-01-30 02:52:30 +0000
From: Hao Wu <<hao.a.wu>>

(In reply to Oleksiy from comment #2)
> Please update status.

Hi Oleksiy,

I think your analysis is right that the EnhancedFatDxe driver misses the
check for the first two fat entries.

Since you already have a fix for this issue here, could you help to
propose a patch to the edk2-devel mailing list? You can refer the detailed
steps at the below link:

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process

Also, for the additional checks on the first & second Fat entry you
proposed, I think it will be better to move them to the function
FatOpenDevice(). There are already some validity check for the volume.

Another thing is that, could you also help to place the same checks at the
FatGetBpbInfo() function within file FatPkg\FatPei\FatLiteAccess.c as well?
The FatPei driver is a simplified version of EnhancedFatDxe for PEI phase.
The same checks can be added as well.

Your contribution will be appreciated.

Best Regards,
Hao Wu

@tianocore-issues
Copy link
Author

Comment 7628

Date: 2019-05-17 13:10:57 +0000
From: shala.arshi

Comment on attachment 245
Fat Boot Sector + Fat Table

comment

Attachment: BZ-245-1.jpg

@tianocore-issues
Copy link
Author

Comment 7630

Date: 2019-05-17 13:11:27 +0000
From: shala.arshi

Created attachment 329
data

comment

Attachment: BZ-329-John

@tianocore-issues
Copy link
Author

Comment 7631

Date: 2019-05-17 13:11:29 +0000
From: shala.arshi

Comment on attachment 245
Fat Boot Sector + Fat Table

comment

Attachment: BZ-245-1.jpg

@tianocore-issues
Copy link
Author

Comment 7638

Date: 2019-05-17 13:12:25 +0000
From: shala.arshi

Comment on attachment 329
data

comment

Attachment: BZ-329-John

@tianocore-issues
Copy link
Author

Comment 7639

Date: 2019-05-17 13:12:30 +0000
From: shala.arshi

Comment on attachment 329
data

comment

Attachment: BZ-329-John

@tianocore-issues
Copy link
Author

Comment 7673

Date: 2019-05-17 15:49:52 +0000
From: Yonghong Zhu <<yonghong.zhu>>

change back to hao.

@tianocore-issues
Copy link
Author

Comment 9375

Date: 2019-08-28 11:44:47 +0000
From: Oleksiy <>

(In reply to Hao Wu from comment #3)
> (In reply to Oleksiy from comment #2)
> > Please update status.
>
> Hi Oleksiy,
>
> I think your analysis is right that the EnhancedFatDxe driver misses the
> check for the first two fat entries.
>
> Since you already have a fix for this issue here, could you help to
> propose a patch to the edk2-devel mailing list? You can refer the detailed
> steps at the below link:
>
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-
> Process
>
> Also, for the additional checks on the first & second Fat entry you
> proposed, I think it will be better to move them to the function
> FatOpenDevice(). There are already some validity check for the volume.
>
> Another thing is that, could you also help to place the same checks at the
> FatGetBpbInfo() function within file FatPkg\FatPei\FatLiteAccess.c as well?
> The FatPei driver is a simplified version of EnhancedFatDxe for PEI phase.
> The same checks can be added as well.
>
> Your contribution will be appreciated.
>
> Best Regards,
> Hao Wu

Hi Hao.

I must miss you comment on this.

Sorry I do not have time, equipment to test and expertise to check/implement your remarks.
If you want you can take my suggestion. If you think it is incomplete/wrong you can address this issue in any other way.
But, please do something with this issue ASAP.

Regards, Oleksiy.

@tianocore-issues
Copy link
Author

Comment 9408

Date: 2019-08-29 23:02:37 +0000
From: Hao Wu <<hao.a.wu>>

Thanks for the information Oleksiy.

I will queue this one into my task.
However, I am sorry that there is no clear timetable for me to dedicate into
this issue. Please expect some delay for the fix.

@tianocore-issues
Copy link
Author

Comment 18305

Date: 2022-02-09 20:39:30 +0000
From: Hao Wu <<hao.a.wu>>

Assignee to unassigned.

os-d added a commit to os-d/edk2 that referenced this issue Jan 10, 2025
There are two reserved FAT entries in the FAT filesystem that
are expected to have valid contents in them. Today the FAT drivers
do not validate these entries when reading from a device for the
first time. This can cause infinite loops in the FAT driver when
trying to read corrupted disks as reported in
tianocore#9679.

This PR follows the recommended update requested in that bug to
check the two reserved FAT entries and validate their contents
against the spec defined values in both FatPei and EnhancedFatDxe
when opening a device for the first time.

Signed-off-by: Oliver Smith-Denny <[email protected]>
@os-d os-d linked a pull request Jan 10, 2025 that will close this issue
3 tasks
os-d added a commit to os-d/edk2 that referenced this issue Jan 10, 2025
There are two reserved FAT entries in the FAT filesystem that
are expected to have valid contents in them. Today the FAT drivers
do not validate these entries when reading from a device for the
first time. This can cause infinite loops in the FAT driver when
trying to read corrupted disks as reported in
tianocore#9679.

This PR follows the recommended update requested in that bug to
check the two reserved FAT entries and validate their contents
against the spec defined values in both FatPei and EnhancedFatDxe
when opening a device for the first time.

Signed-off-by: Oliver Smith-Denny <[email protected]>
@os-d os-d linked a pull request Jan 10, 2025 that will close this issue
3 tasks
os-d added a commit to os-d/edk2 that referenced this issue Jan 13, 2025
There are two reserved FAT entries in the FAT filesystem that
are expected to have valid contents in them. Today the FAT drivers
do not validate these entries when reading from a device for the
first time. This can cause infinite loops in the FAT driver when
trying to read corrupted disks as reported in
tianocore#9679.

This PR follows the recommended update requested in that bug to
check the two reserved FAT entries and validate their contents
against the spec defined values in both FatPei and EnhancedFatDxe
when opening a device for the first time.

Signed-off-by: Oliver Smith-Denny <[email protected]>
os-d added a commit to os-d/edk2 that referenced this issue Jan 15, 2025
There are two reserved FAT entries in the FAT filesystem that
are expected to have valid contents in them. Today the FAT drivers
do not validate these entries when reading from a device for the
first time. This can cause infinite loops in the FAT driver when
trying to read corrupted disks as reported in
tianocore#9679.

This PR follows the recommended update requested in that bug to
check the two reserved FAT entries and validate their contents
against the spec defined values in both FatPei and EnhancedFatDxe
when opening a device for the first time.

Signed-off-by: Oliver Smith-Denny <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:fatpkg priority:high Significant impact. Should be fixed as soon as possible. type:bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant