On 08/09/2017 05:35, Laszlo Ersek wrote:
Paulo,
On 09/08/17 02:56, Paulo Alcantara wrote:
Ray,
On 07/09/2017 20:13, Paulo Alcantara wrote:
v5:
- Fixed OVMF IA32 build.
- Fixed a typo in UdfDriveBindingStop() ("This" -> "SimpleFs") which
broke retrieval of private fs data from SimpleFs protocol --
identified by 'reconnect -r' command in UEFI shell.
Follow the diff between v4 and v5 for Mde*Pkg changes (forgot to include
it when resending):
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
index 8ad14fe594..2dbcff0be4 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
@@ -372,7 +372,7 @@ UdfRead (
PrivFileData->FileSize,
&PrivFileData->FilePosition,
Buffer,
- BufferSize
+ (UINT64 *)(UINTN)BufferSize^M
);
This change is not correct.
(1) The UdfRead() function takes the following parameter:
IN OUT UINTN *BufferSize,
This means that, in an IA32 or ARM build,
sizeof *BufferSize == 4
and in an AARCH64 or X64 build,
sizeof *BufferSize == 8
(2) The above type-casting is part of a call to the ReadFileData()
function. The ReadFileData() function takes the following parameter:
IN OUT UINT64 *BufferSize
This means that, regardless of architecture,
sizeof *BufferSize == 8
The consequence is that, in an IA32 or ARM build, the ReadFileData()
function will both read and write beyond the end of the outermost
caller's "BufferSize" variable. The write is a problem without a doubt,
but the read is a problem too if the outermost caller's "BufferSize" (a
UINT32 object) is followed by four bytes that are not all zero. Then
ReadFileData() will attempt to read more than 4GB of data.
You're right. Thanks for the explanation.
The right way to fix this is the following:
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
index 2dbcff0be4a3..07c7ec207fcd 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
@@ -326,6 +326,7 @@ UdfRead (
VOID *NewFileEntryData;
CHAR16 FileName[UDF_FILENAME_LENGTH] = { 0 };
UINT64 FileSize;
+ UINT64 BufferSizeUint64;
OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
@@ -364,6 +365,7 @@ UdfRead (
goto Done;
}
+ BufferSizeUint64 = *BufferSize;
Status = ReadFileData (
BlockIo,
DiskIo,
@@ -372,8 +374,10 @@ UdfRead (
PrivFileData->FileSize,
&PrivFileData->FilePosition,
Buffer,
- (UINT64 *)(UINTN)BufferSize
+ &BufferSizeUint64
);
+ ASSERT (BufferSizeUint64 <= MAX_UINTN);
+ *BufferSize = (UINTN)BufferSizeUint64;
} else if (IS_FID_DIRECTORY_FILE (Parent->FileIdentifierDesc)) {
if (ReadDirInfo->FidOffset == 0 && PrivFileData->FilePosition > 0) {
Status = EFI_DEVICE_ERROR;
I'll include this in v6.
Paulo
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
