* [PATCH v1 01/10] MdeModulePkg/PartitionDxe: Add check for underlying device block size
2018-10-16 7:23 [PATCH v1 00/10] UDF: Bugfixes Hao Wu
@ 2018-10-16 7:23 ` Hao Wu
2018-10-16 7:23 ` [PATCH v1 02/10] MdeModulePkg/UdfDxe: Refine boundary checks for file/path name string Hao Wu
` (9 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Hao Wu @ 2018-10-16 7:23 UTC (permalink / raw)
To: edk2-devel; +Cc: Hao Wu, Paulo Alcantara, Ruiyu Ni, Jiewen Yao, Star Zeng
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=828
Within FindAnchorVolumeDescriptorPointer():
Add a check for the underlying device block size to ensure it is greater
than the size of an Anchor Volume Descriptor Pointer.
Cc: Paulo Alcantara <paulo@paulo.ac>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 28 ++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
index 83bd174231..0fd56b75b4 100644
--- a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
+++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
@@ -1,6 +1,14 @@
/** @file
Scan for an UDF file system on a formatted media.
+ Caution: This file requires additional review when modified.
+ This driver will have external input - CD/DVD media.
+ This external input must be validated carefully to avoid security issue like
+ buffer overflow, integer overflow.
+
+ FindUdfFileSystem() routine will consume the media properties and do basic
+ validations.
+
Copyright (c) 2018 Qualcomm Datacenter Technologies, Inc.
Copyright (C) 2014-2017 Paulo Alcantara <pcacjr@zytor.com>
@@ -102,6 +110,20 @@ FindAnchorVolumeDescriptorPointer (
AvdpsCount = 0;
//
+ // Check if the block size of the underlying media can hold the data of an
+ // Anchor Volume Descriptor Pointer
+ //
+ if (BlockSize < sizeof (UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER)) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "%a: Media block size 0x%x unable to hold an AVDP.\n",
+ __FUNCTION__,
+ BlockSize
+ ));
+ return EFI_UNSUPPORTED;
+ }
+
+ //
// Find AVDP at block 256
//
Status = DiskIo->ReadDisk (
@@ -598,6 +620,12 @@ Out_Free:
/**
Find a supported UDF file system in block device.
+ @attention This is boundary function that may receive untrusted input.
+ @attention The input is from Partition.
+
+ The CD/DVD media is the external input, so this routine will do basic
+ validations for the media.
+
@param[in] BlockIo BlockIo interface.
@param[in] DiskIo DiskIo interface.
@param[out] StartingLBA UDF file system starting LBA.
--
2.12.0.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v1 02/10] MdeModulePkg/UdfDxe: Refine boundary checks for file/path name string
2018-10-16 7:23 [PATCH v1 00/10] UDF: Bugfixes Hao Wu
2018-10-16 7:23 ` [PATCH v1 01/10] MdeModulePkg/PartitionDxe: Add check for underlying device block size Hao Wu
@ 2018-10-16 7:23 ` Hao Wu
2018-10-16 7:23 ` [PATCH v1 03/10] MdeModulePkg/UdfDxe: Add boundary check the read of FE/EFE Hao Wu
` (8 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Hao Wu @ 2018-10-16 7:23 UTC (permalink / raw)
To: edk2-devel; +Cc: Hao Wu, Paulo Alcantara, Ruiyu Ni, Jiewen Yao, Star Zeng
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=828
The commit refines the boundary checks for file/path name string to
prevent possible buffer overrun.
Cc: Paulo Alcantara <paulo@paulo.ac>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
MdeModulePkg/Universal/Disk/UdfDxe/File.c | 29 +++++++--
MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 64 +++++++++++++++++---
MdeModulePkg/Universal/Disk/UdfDxe/Udf.h | 29 ++++++++-
3 files changed, 107 insertions(+), 15 deletions(-)
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
index 6f07bf2066..80db734f86 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
@@ -248,7 +248,7 @@ UdfOpen (
FileName = TempFileName + 1;
}
- StrCpyS (NewPrivFileData->FileName, UDF_PATH_LENGTH, FileName);
+ StrCpyS (NewPrivFileData->FileName, UDF_FILENAME_LENGTH, FileName);
Status = GetFileSize (
PrivFsData->BlockIo,
@@ -444,7 +444,7 @@ UdfRead (
FreePool ((VOID *)NewFileEntryData);
NewFileEntryData = FoundFile.FileEntry;
- Status = GetFileNameFromFid (NewFileIdentifierDesc, FileName);
+ Status = GetFileNameFromFid (NewFileIdentifierDesc, ARRAY_SIZE (FileName), FileName);
if (EFI_ERROR (Status)) {
FreePool ((VOID *)FoundFile.FileIdentifierDesc);
goto Error_Get_FileName;
@@ -456,7 +456,7 @@ UdfRead (
FoundFile.FileIdentifierDesc = NewFileIdentifierDesc;
FoundFile.FileEntry = NewFileEntryData;
- Status = GetFileNameFromFid (FoundFile.FileIdentifierDesc, FileName);
+ Status = GetFileNameFromFid (FoundFile.FileIdentifierDesc, ARRAY_SIZE (FileName), FileName);
if (EFI_ERROR (Status)) {
goto Error_Get_FileName;
}
@@ -718,6 +718,12 @@ UdfSetPosition (
/**
Get information about a file.
+ @attention This is boundary function that may receive untrusted input.
+ @attention The input is from FileSystem.
+
+ The File Set Descriptor is external input, so this routine will do basic
+ validation for File Set Descriptor and report status.
+
@param This Protocol instance pointer.
@param InformationType Type of information to return in Buffer.
@param BufferSize On input size of buffer, on output amount of data in
@@ -794,6 +800,10 @@ UdfGetInfo (
*String = *(UINT8 *)(OstaCompressed + Index) << 8;
Index++;
} else {
+ if (Index > ARRAY_SIZE (VolumeLabel)) {
+ return EFI_VOLUME_CORRUPTED;
+ }
+
*String = 0;
}
@@ -813,7 +823,11 @@ UdfGetInfo (
String++;
}
- *String = L'\0';
+ Index = ((UINTN)String - (UINTN)VolumeLabel) / sizeof (CHAR16);
+ if (Index > ARRAY_SIZE (VolumeLabel) - 1) {
+ Index = ARRAY_SIZE (VolumeLabel) - 1;
+ }
+ VolumeLabel[Index] = L'\0';
FileSystemInfoLength = StrSize (VolumeLabel) +
sizeof (EFI_FILE_SYSTEM_INFO);
@@ -823,8 +837,11 @@ UdfGetInfo (
}
FileSystemInfo = (EFI_FILE_SYSTEM_INFO *)Buffer;
- StrCpyS (FileSystemInfo->VolumeLabel, ARRAY_SIZE (VolumeLabel),
- VolumeLabel);
+ StrCpyS (
+ FileSystemInfo->VolumeLabel,
+ (*BufferSize - OFFSET_OF (EFI_FILE_SYSTEM_INFO, VolumeLabel)) / sizeof (CHAR16),
+ VolumeLabel
+ );
Status = GetVolumeSize (
PrivFsData->BlockIo,
PrivFsData->DiskIo,
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index ecc172303e..5fb88c2ff3 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -1412,7 +1412,7 @@ InternalFindFile (
break;
}
} else {
- Status = GetFileNameFromFid (FileIdentifierDesc, FoundFileName);
+ Status = GetFileNameFromFid (FileIdentifierDesc, ARRAY_SIZE (FoundFileName), FoundFileName);
if (EFI_ERROR (Status)) {
break;
}
@@ -1705,6 +1705,11 @@ FindFile (
while (*FilePath != L'\0') {
FileNamePointer = FileName;
while (*FilePath != L'\0' && *FilePath != L'\\') {
+ if ((((UINTN)FileNamePointer - (UINTN)FileName) / sizeof (CHAR16)) >=
+ (ARRAY_SIZE (FileName) - 1)) {
+ return EFI_NOT_FOUND;
+ }
+
*FileNamePointer++ = *FilePath++;
}
@@ -1882,22 +1887,38 @@ ReadDirectoryEntry (
Get a filename (encoded in OSTA-compressed format) from a File Identifier
Descriptor on an UDF volume.
+ @attention This is boundary function that may receive untrusted input.
+ @attention The input is from FileSystem.
+
+ The File Identifier Descriptor is external input, so this routine will do
+ basic validation for File Identifier Descriptor and report status.
+
@param[in] FileIdentifierDesc File Identifier Descriptor pointer.
+ @param[in] CharMax The maximum number of FileName Unicode char,
+ including terminating null char.
@param[out] FileName Decoded filename.
@retval EFI_SUCCESS Filename decoded and read.
@retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
+ @retval EFI_BUFFER_TOO_SMALL The string buffer FileName cannot hold the
+ decoded filename.
**/
EFI_STATUS
GetFileNameFromFid (
IN UDF_FILE_IDENTIFIER_DESCRIPTOR *FileIdentifierDesc,
+ IN UINTN CharMax,
OUT CHAR16 *FileName
)
{
- UINT8 *OstaCompressed;
- UINT8 CompressionId;
- UINT8 Length;
- UINTN Index;
+ UINT8 *OstaCompressed;
+ UINT8 CompressionId;
+ UINT8 Length;
+ UINTN Index;
+ CHAR16 *FileNameBak;
+
+ if (CharMax == 0) {
+ return EFI_BUFFER_TOO_SMALL;
+ }
OstaCompressed =
(UINT8 *)(
@@ -1910,10 +1931,22 @@ GetFileNameFromFid (
return EFI_VOLUME_CORRUPTED;
}
+ FileNameBak = FileName;
+
//
// Decode filename.
//
Length = FileIdentifierDesc->LengthOfFileIdentifier;
+ if (CompressionId == 16) {
+ if (((UINTN)Length >> 1) > CharMax) {
+ return EFI_BUFFER_TOO_SMALL;
+ }
+ } else {
+ if ((Length != 0) && ((UINTN)Length - 1 > CharMax)) {
+ return EFI_BUFFER_TOO_SMALL;
+ }
+ }
+
for (Index = 1; Index < Length; Index++) {
if (CompressionId == 16) {
*FileName = OstaCompressed[Index++] << 8;
@@ -1928,7 +1961,11 @@ GetFileNameFromFid (
FileName++;
}
- *FileName = L'\0';
+ Index = ((UINTN)FileName - (UINTN)FileNameBak) / sizeof (CHAR16);
+ if (Index > CharMax - 1) {
+ Index = CharMax - 1;
+ }
+ FileNameBak[Index] = L'\0';
return EFI_SUCCESS;
}
@@ -1936,6 +1973,12 @@ GetFileNameFromFid (
/**
Resolve a symlink file on an UDF volume.
+ @attention This is boundary function that may receive untrusted input.
+ @attention The input is from FileSystem.
+
+ The Path Component is external input, so this routine will do basic
+ validation for Path Component and report status.
+
@param[in] BlockIo BlockIo interface.
@param[in] DiskIo DiskIo interface.
@param[in] Volume UDF volume information structure.
@@ -2054,6 +2097,9 @@ ResolveSymlink (
Index) << 8;
Index++;
} else {
+ if (Index > ARRAY_SIZE (FileName)) {
+ return EFI_UNSUPPORTED;
+ }
*Char = 0;
}
@@ -2064,7 +2110,11 @@ ResolveSymlink (
Char++;
}
- *Char = L'\0';
+ Index = ((UINTN)Char - (UINTN)FileName) / sizeof (CHAR16);
+ if (Index > ARRAY_SIZE (FileName) - 1) {
+ Index = ARRAY_SIZE (FileName) - 1;
+ }
+ FileName[Index] = L'\0';
break;
}
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/Udf.h b/MdeModulePkg/Universal/Disk/UdfDxe/Udf.h
index d441539d16..85bf5e7733 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/Udf.h
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/Udf.h
@@ -559,9 +559,16 @@ UdfSetPosition (
/**
Get information about a file.
+ @attention This is boundary function that may receive untrusted input.
+ @attention The input is from FileSystem.
+
+ The File Set Descriptor is external input, so this routine will do basic
+ validation for File Set Descriptor and report status.
+
@param This Protocol instance pointer.
@param InformationType Type of information to return in Buffer.
- @param BufferSize On input size of buffer, on output amount of data in buffer.
+ @param BufferSize On input size of buffer, on output amount of data in
+ buffer.
@param Buffer The buffer to return data.
@retval EFI_SUCCESS Data was returned.
@@ -571,7 +578,8 @@ UdfSetPosition (
@retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
@retval EFI_WRITE_PROTECTED The device is write protected.
@retval EFI_ACCESS_DENIED The file was open for read only.
- @retval EFI_BUFFER_TOO_SMALL Buffer was too small; required size returned in BufferSize.
+ @retval EFI_BUFFER_TOO_SMALL Buffer was too small; required size returned in
+ BufferSize.
**/
EFI_STATUS
@@ -769,21 +777,38 @@ ReadDirectoryEntry (
Get a filename (encoded in OSTA-compressed format) from a File Identifier
Descriptor on an UDF volume.
+ @attention This is boundary function that may receive untrusted input.
+ @attention The input is from FileSystem.
+
+ The File Identifier Descriptor is external input, so this routine will do
+ basic validation for File Identifier Descriptor and report status.
+
@param[in] FileIdentifierDesc File Identifier Descriptor pointer.
+ @param[in] CharMax The maximum number of FileName Unicode char,
+ including terminating null char.
@param[out] FileName Decoded filename.
@retval EFI_SUCCESS Filename decoded and read.
@retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
+ @retval EFI_BUFFER_TOO_SMALL The string buffer FileName cannot hold the
+ decoded filename.
**/
EFI_STATUS
GetFileNameFromFid (
IN UDF_FILE_IDENTIFIER_DESCRIPTOR *FileIdentifierDesc,
+ IN UINTN CharMax,
OUT CHAR16 *FileName
);
/**
Resolve a symlink file on an UDF volume.
+ @attention This is boundary function that may receive untrusted input.
+ @attention The input is from FileSystem.
+
+ The Path Component is external input, so this routine will do basic
+ validation for Path Component and report status.
+
@param[in] BlockIo BlockIo interface.
@param[in] DiskIo DiskIo interface.
@param[in] Volume UDF volume information structure.
--
2.12.0.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v1 03/10] MdeModulePkg/UdfDxe: Add boundary check the read of FE/EFE
2018-10-16 7:23 [PATCH v1 00/10] UDF: Bugfixes Hao Wu
2018-10-16 7:23 ` [PATCH v1 01/10] MdeModulePkg/PartitionDxe: Add check for underlying device block size Hao Wu
2018-10-16 7:23 ` [PATCH v1 02/10] MdeModulePkg/UdfDxe: Refine boundary checks for file/path name string Hao Wu
@ 2018-10-16 7:23 ` Hao Wu
2018-10-16 7:23 ` [PATCH v1 04/10] MdeModulePkg/UdfDxe: Add boundary check for ComponentIdentifier decode Hao Wu
` (7 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Hao Wu @ 2018-10-16 7:23 UTC (permalink / raw)
To: edk2-devel; +Cc: Hao Wu, Paulo Alcantara, Ruiyu Ni, Jiewen Yao, Star Zeng
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=828
Within ReadFile():
Add checks to ensure that when getting the raw data or the Allocation
Descriptors' data from a FE/EFE, it will not consume data beyond the
size of a FE/EFE.
Cc: Paulo Alcantara <paulo@paulo.ac>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 54 ++++++++++++++++++--
1 file changed, 50 insertions(+), 4 deletions(-)
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 5fb88c2ff3..d758b798f1 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -503,15 +503,27 @@ DuplicateFe (
NOTE: The FE/EFE can be thought it was an inode.
+ @attention This is boundary function that may receive untrusted input.
+ @attention The input is from FileSystem.
+
+ The (Extended) File Entry is external input, so this routine will do basic
+ validation for (Extended) File Entry and report status.
+
@param[in] FileEntryData (Extended) File Entry pointer.
+ @param[in] FileEntrySize Size of the (Extended) File Entry specified
+ by FileEntryData.
@param[out] Data Buffer contains the raw data of a given
(Extended) File Entry.
@param[out] Length Length of the data in Buffer.
+ @retval EFI_SUCCESS Raw data and size of the FE/EFE was read.
+ @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
+
**/
-VOID
+EFI_STATUS
GetFileEntryData (
IN VOID *FileEntryData,
+ IN UINTN FileEntrySize,
OUT VOID **Data,
OUT UINT64 *Length
)
@@ -535,20 +547,40 @@ GetFileEntryData (
*Data = (VOID *)((UINT8 *)FileEntry->Data +
FileEntry->LengthOfExtendedAttributes);
}
+
+ if ((*Length > FileEntrySize) ||
+ ((UINTN)FileEntryData > (UINTN)(*Data)) ||
+ ((UINTN)(*Data) - (UINTN)FileEntryData > FileEntrySize - *Length)) {
+ return EFI_VOLUME_CORRUPTED;
+ }
+ return EFI_SUCCESS;
}
/**
Get Allocation Descriptors' data information from a given FE/EFE.
+ @attention This is boundary function that may receive untrusted input.
+ @attention The input is from FileSystem.
+
+ The (Extended) File Entry is external input, so this routine will do basic
+ validation for (Extended) File Entry and report status.
+
@param[in] FileEntryData (Extended) File Entry pointer.
+ @param[in] FileEntrySize Size of the (Extended) File Entry specified
+ by FileEntryData.
@param[out] AdsData Buffer contains the Allocation Descriptors'
data from a given FE/EFE.
@param[out] Length Length of the data in AdsData.
+ @retval EFI_SUCCESS The data and size of Allocation Descriptors
+ were read from the FE/EFE.
+ @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
+
**/
-VOID
+EFI_STATUS
GetAdsInformation (
IN VOID *FileEntryData,
+ IN UINTN FileEntrySize,
OUT VOID **AdsData,
OUT UINT64 *Length
)
@@ -572,6 +604,13 @@ GetAdsInformation (
*AdsData = (VOID *)((UINT8 *)FileEntry->Data +
FileEntry->LengthOfExtendedAttributes);
}
+
+ if ((*Length > FileEntrySize) ||
+ ((UINTN)FileEntryData > (UINTN)(*AdsData)) ||
+ ((UINTN)(*AdsData) - (UINTN)FileEntryData > FileEntrySize - *Length)) {
+ return EFI_VOLUME_CORRUPTED;
+ }
+ return EFI_SUCCESS;
}
/**
@@ -1065,7 +1104,10 @@ ReadFile (
//
// There are no extents for this FE/EFE. All data is inline.
//
- GetFileEntryData (FileEntryData, &Data, &Length);
+ Status = GetFileEntryData (FileEntryData, Volume->FileEntrySize, &Data, &Length);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
if (ReadFileInfo->Flags == ReadFileGetFileSize) {
ReadFileInfo->ReadLength = Length;
@@ -1109,7 +1151,11 @@ ReadFile (
// This FE/EFE contains a run of Allocation Descriptors. Get data + size
// for start reading them out.
//
- GetAdsInformation (FileEntryData, &Data, &Length);
+ Status = GetAdsInformation (FileEntryData, Volume->FileEntrySize, &Data, &Length);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
AdOffset = 0;
for (;;) {
--
2.12.0.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v1 04/10] MdeModulePkg/UdfDxe: Add boundary check for ComponentIdentifier decode
2018-10-16 7:23 [PATCH v1 00/10] UDF: Bugfixes Hao Wu
` (2 preceding siblings ...)
2018-10-16 7:23 ` [PATCH v1 03/10] MdeModulePkg/UdfDxe: Add boundary check the read of FE/EFE Hao Wu
@ 2018-10-16 7:23 ` Hao Wu
2018-10-16 7:23 ` [PATCH v1 05/10] MdeModulePkg/UdfDxe: Add boundary check for getting volume (free) size Hao Wu
` (6 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Hao Wu @ 2018-10-16 7:23 UTC (permalink / raw)
To: edk2-devel; +Cc: Hao Wu, Paulo Alcantara, Ruiyu Ni, Jiewen Yao, Star Zeng
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=828
Within ResolveSymlink():
The boundary check will validate the 'LengthofComponentIdentifier' field
of a Path Component matches the data within the relating (Extended) File
Entry.
Cc: Paulo Alcantara <paulo@paulo.ac>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index d758b798f1..7611d28b5a 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -2136,6 +2136,10 @@ ResolveSymlink (
return EFI_VOLUME_CORRUPTED;
}
+ if ((UINTN)PathComp->ComponentIdentifier + PathCompLength > (UINTN)EndData) {
+ return EFI_VOLUME_CORRUPTED;
+ }
+
Char = FileName;
for (Index = 1; Index < PathCompLength; Index++) {
if (CompressionId == 16) {
--
2.12.0.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v1 05/10] MdeModulePkg/UdfDxe: Add boundary check for getting volume (free) size
2018-10-16 7:23 [PATCH v1 00/10] UDF: Bugfixes Hao Wu
` (3 preceding siblings ...)
2018-10-16 7:23 ` [PATCH v1 04/10] MdeModulePkg/UdfDxe: Add boundary check for ComponentIdentifier decode Hao Wu
@ 2018-10-16 7:23 ` Hao Wu
2018-10-16 7:23 ` [PATCH v1 06/10] MdeModulePkg/UdfDxe: Correct behavior for UdfSetPosition() Hao Wu
` (5 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Hao Wu @ 2018-10-16 7:23 UTC (permalink / raw)
To: edk2-devel; +Cc: Hao Wu, Paulo Alcantara, Ruiyu Ni, Jiewen Yao, Star Zeng
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=828
Within GetVolumeSize():
The boundary check will validate the 'NumberOfPartitions' field of a
Logical Volume Integrity Descriptor matches the data within the relating
Logical Volume Descriptor.
Cc: Paulo Alcantara <paulo@paulo.ac>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 17 ++++++++++++++++-
MdeModulePkg/Universal/Disk/UdfDxe/Udf.h | 7 +++++++
2 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 7611d28b5a..826ffccf81 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -2450,6 +2450,13 @@ SetFileInfo (
/**
Get volume and free space size information of an UDF volume.
+ @attention This is boundary function that may receive untrusted input.
+ @attention The input is from FileSystem.
+
+ The Logical Volume Descriptor and the Logical Volume Integrity Descriptor are
+ external inputs, so this routine will do basic validation for both descriptors
+ and report status.
+
@param[in] BlockIo BlockIo interface.
@param[in] DiskIo DiskIo interface.
@param[in] Volume UDF volume information structure.
@@ -2488,7 +2495,8 @@ GetVolumeSize (
ExtentAd = &LogicalVolDesc->IntegritySequenceExtent;
- if (ExtentAd->ExtentLength == 0) {
+ if ((ExtentAd->ExtentLength == 0) ||
+ (ExtentAd->ExtentLength < sizeof (UDF_LOGICAL_VOLUME_INTEGRITY))) {
return EFI_VOLUME_CORRUPTED;
}
@@ -2528,6 +2536,13 @@ GetVolumeSize (
goto Out_Free;
}
+ if ((LogicalVolInt->NumberOfPartitions > MAX_UINT32 / sizeof (UINT32) / 2) ||
+ (LogicalVolInt->NumberOfPartitions * sizeof (UINT32) * 2 >
+ ExtentAd->ExtentLength - sizeof (UDF_LOGICAL_VOLUME_INTEGRITY))) {
+ Status = EFI_VOLUME_CORRUPTED;
+ goto Out_Free;
+ }
+
*VolumeSize = 0;
*FreeSpaceSize = 0;
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/Udf.h b/MdeModulePkg/Universal/Disk/UdfDxe/Udf.h
index 85bf5e7733..9c3f21fd05 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/Udf.h
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/Udf.h
@@ -902,6 +902,13 @@ SetFileInfo (
/**
Get volume and free space size information of an UDF volume.
+ @attention This is boundary function that may receive untrusted input.
+ @attention The input is from FileSystem.
+
+ The Logical Volume Descriptor and the Logical Volume Integrity Descriptor are
+ external inputs, so this routine will do basic validation for both descriptors
+ and report status.
+
@param[in] BlockIo BlockIo interface.
@param[in] DiskIo DiskIo interface.
@param[in] Volume UDF volume information structure.
--
2.12.0.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v1 06/10] MdeModulePkg/UdfDxe: Correct behavior for UdfSetPosition()
2018-10-16 7:23 [PATCH v1 00/10] UDF: Bugfixes Hao Wu
` (4 preceding siblings ...)
2018-10-16 7:23 ` [PATCH v1 05/10] MdeModulePkg/UdfDxe: Add boundary check for getting volume (free) size Hao Wu
@ 2018-10-16 7:23 ` Hao Wu
2018-10-16 7:23 ` [PATCH v1 07/10] MdeModulePkg/UdfDxe: Fix a typo within SetFileInfo() Hao Wu
` (4 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Hao Wu @ 2018-10-16 7:23 UTC (permalink / raw)
To: edk2-devel; +Cc: Hao Wu, Paulo Alcantara, Ruiyu Ni, Star Zeng
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1252
Calling the 'SetPosition' service of the EFI_FILE_PROTOCOL with 'Position'
equals to 0xFFFFFFFFFFFFFFFF for a file is to set the current position to
the end of the file. But the current implementation of function
UdfSetPosition() is to set it to the last byte (not EOF).
This commit will resolve this issue.
Cc: Paulo Alcantara <paulo@paulo.ac>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
MdeModulePkg/Universal/Disk/UdfDxe/File.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
index 80db734f86..54c2400243 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
@@ -704,7 +704,7 @@ UdfSetPosition (
// set to the EOF.
//
if (Position == 0xFFFFFFFFFFFFFFFF) {
- PrivFileData->FilePosition = PrivFileData->FileSize - 1;
+ PrivFileData->FilePosition = PrivFileData->FileSize;
} else {
PrivFileData->FilePosition = Position;
}
--
2.12.0.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v1 07/10] MdeModulePkg/UdfDxe: Fix a typo within SetFileInfo()
2018-10-16 7:23 [PATCH v1 00/10] UDF: Bugfixes Hao Wu
` (5 preceding siblings ...)
2018-10-16 7:23 ` [PATCH v1 06/10] MdeModulePkg/UdfDxe: Correct behavior for UdfSetPosition() Hao Wu
@ 2018-10-16 7:23 ` Hao Wu
2018-10-16 7:23 ` [PATCH v1 08/10] MdeModulePkg/UdfDxe: Update GetInfo() for FS VolumeLabel info request Hao Wu
` (3 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Hao Wu @ 2018-10-16 7:23 UTC (permalink / raw)
To: edk2-devel; +Cc: Hao Wu, Paulo Alcantara, Ruiyu Ni, Star Zeng
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1253
Within function SetFileInfo():
This commit will fix a typo where 'Minute' should be used instead of
'Second'.
Cc: Paulo Alcantara <paulo@paulo.ac>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 826ffccf81..ac6e0a8ff7 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -2370,7 +2370,7 @@ SetFileInfo (
FileInfo->CreateTime.Month = FileEntry->AccessTime.Month;
FileInfo->CreateTime.Day = FileEntry->AccessTime.Day;
FileInfo->CreateTime.Hour = FileEntry->AccessTime.Hour;
- FileInfo->CreateTime.Minute = FileEntry->AccessTime.Second;
+ FileInfo->CreateTime.Minute = FileEntry->AccessTime.Minute;
FileInfo->CreateTime.Second = FileEntry->AccessTime.Second;
FileInfo->CreateTime.Nanosecond =
FileEntry->AccessTime.HundredsOfMicroseconds;
--
2.12.0.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v1 08/10] MdeModulePkg/UdfDxe: Update GetInfo() for FS VolumeLabel info request
2018-10-16 7:23 [PATCH v1 00/10] UDF: Bugfixes Hao Wu
` (6 preceding siblings ...)
2018-10-16 7:23 ` [PATCH v1 07/10] MdeModulePkg/UdfDxe: Fix a typo within SetFileInfo() Hao Wu
@ 2018-10-16 7:23 ` Hao Wu
2018-10-16 7:23 ` [PATCH v1 09/10] MdeModulePkg/UdfDxe: Add more check when getting PD from LongAd Hao Wu
` (2 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Hao Wu @ 2018-10-16 7:23 UTC (permalink / raw)
To: edk2-devel; +Cc: Hao Wu, Paulo Alcantara, Ruiyu Ni, Star Zeng
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1175
This commit will update the UdfGetInfo() function with the support of
EFI_FILE_SYSTEM_VOLUME_LABEL data information request.
Cc: Paulo Alcantara <paulo@paulo.ac>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
MdeModulePkg/Universal/Disk/UdfDxe/File.c | 97 +++++++-------------
MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 84 +++++++++++++++++
MdeModulePkg/Universal/Disk/UdfDxe/Udf.h | 27 ++++++
MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf | 1 +
4 files changed, 146 insertions(+), 63 deletions(-)
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
index 54c2400243..eb91e877ee 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
@@ -718,12 +718,6 @@ UdfSetPosition (
/**
Get information about a file.
- @attention This is boundary function that may receive untrusted input.
- @attention The input is from FileSystem.
-
- The File Set Descriptor is external input, so this routine will do basic
- validation for File Set Descriptor and report status.
-
@param This Protocol instance pointer.
@param InformationType Type of information to return in Buffer.
@param BufferSize On input size of buffer, on output amount of data in
@@ -750,19 +744,16 @@ UdfGetInfo (
OUT VOID *Buffer
)
{
- EFI_STATUS Status;
- PRIVATE_UDF_FILE_DATA *PrivFileData;
- PRIVATE_UDF_SIMPLE_FS_DATA *PrivFsData;
- EFI_FILE_SYSTEM_INFO *FileSystemInfo;
- UINTN FileSystemInfoLength;
- CHAR16 *String;
- UDF_FILE_SET_DESCRIPTOR *FileSetDesc;
- UINTN Index;
- UINT8 *OstaCompressed;
- UINT8 CompressionId;
- UINT64 VolumeSize;
- UINT64 FreeSpaceSize;
- CHAR16 VolumeLabel[64];
+ EFI_STATUS Status;
+ PRIVATE_UDF_FILE_DATA *PrivFileData;
+ PRIVATE_UDF_SIMPLE_FS_DATA *PrivFsData;
+ EFI_FILE_SYSTEM_INFO *FileSystemInfo;
+ UINTN FileSystemInfoLength;
+ UINT64 VolumeSize;
+ UINT64 FreeSpaceSize;
+ EFI_FILE_SYSTEM_VOLUME_LABEL *FileSystemVolumeLabel;
+ UINTN FileSystemVolumeLabelLength;
+ CHAR16 VolumeLabel[64];
if (This == NULL || InformationType == NULL || BufferSize == NULL ||
(*BufferSize != 0 && Buffer == NULL)) {
@@ -784,50 +775,10 @@ UdfGetInfo (
Buffer
);
} else if (CompareGuid (InformationType, &gEfiFileSystemInfoGuid)) {
- String = VolumeLabel;
-
- FileSetDesc = &PrivFsData->Volume.FileSetDesc;
-
- OstaCompressed = &FileSetDesc->LogicalVolumeIdentifier[0];
-
- CompressionId = OstaCompressed[0];
- if (!IS_VALID_COMPRESSION_ID (CompressionId)) {
- return EFI_VOLUME_CORRUPTED;
- }
-
- for (Index = 1; Index < 128; Index++) {
- if (CompressionId == 16) {
- *String = *(UINT8 *)(OstaCompressed + Index) << 8;
- Index++;
- } else {
- if (Index > ARRAY_SIZE (VolumeLabel)) {
- return EFI_VOLUME_CORRUPTED;
- }
-
- *String = 0;
- }
-
- if (Index < 128) {
- *String |= (CHAR16)(*(UINT8 *)(OstaCompressed + Index));
- }
-
- //
- // Unlike FID Identifiers, Logical Volume Identifier is stored in a
- // NULL-terminated OSTA compressed format, so we must check for the NULL
- // character.
- //
- if (*String == L'\0') {
- break;
- }
-
- String++;
- }
-
- Index = ((UINTN)String - (UINTN)VolumeLabel) / sizeof (CHAR16);
- if (Index > ARRAY_SIZE (VolumeLabel) - 1) {
- Index = ARRAY_SIZE (VolumeLabel) - 1;
+ Status = GetVolumeLabel (&PrivFsData->Volume, ARRAY_SIZE (VolumeLabel), VolumeLabel);
+ if (EFI_ERROR (Status)) {
+ return Status;
}
- VolumeLabel[Index] = L'\0';
FileSystemInfoLength = StrSize (VolumeLabel) +
sizeof (EFI_FILE_SYSTEM_INFO);
@@ -839,7 +790,7 @@ UdfGetInfo (
FileSystemInfo = (EFI_FILE_SYSTEM_INFO *)Buffer;
StrCpyS (
FileSystemInfo->VolumeLabel,
- (*BufferSize - OFFSET_OF (EFI_FILE_SYSTEM_INFO, VolumeLabel)) / sizeof (CHAR16),
+ (*BufferSize - SIZE_OF_EFI_FILE_SYSTEM_INFO) / sizeof (CHAR16),
VolumeLabel
);
Status = GetVolumeSize (
@@ -862,6 +813,26 @@ UdfGetInfo (
*BufferSize = FileSystemInfoLength;
Status = EFI_SUCCESS;
+ } else if (CompareGuid (InformationType, &gEfiFileSystemVolumeLabelInfoIdGuid)) {
+ Status = GetVolumeLabel (&PrivFsData->Volume, ARRAY_SIZE (VolumeLabel), VolumeLabel);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ FileSystemVolumeLabelLength = StrSize (VolumeLabel) +
+ sizeof (EFI_FILE_SYSTEM_VOLUME_LABEL);
+ if (*BufferSize < FileSystemVolumeLabelLength) {
+ *BufferSize = FileSystemVolumeLabelLength;
+ return EFI_BUFFER_TOO_SMALL;
+ }
+
+ FileSystemVolumeLabel = (EFI_FILE_SYSTEM_VOLUME_LABEL *)Buffer;
+ StrCpyS (
+ FileSystemVolumeLabel->VolumeLabel,
+ (*BufferSize - SIZE_OF_EFI_FILE_SYSTEM_VOLUME_LABEL) / sizeof (CHAR16),
+ VolumeLabel
+ );
+ Status = EFI_SUCCESS;
}
return Status;
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index ac6e0a8ff7..562a7d983c 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -2448,6 +2448,90 @@ SetFileInfo (
}
/**
+ Get volume label of an UDF volume.
+
+ @attention This is boundary function that may receive untrusted input.
+ @attention The input is from FileSystem.
+
+ The File Set Descriptor is external input, so this routine will do basic
+ validation for File Set Descriptor and report status.
+
+ @param[in] Volume Volume information pointer.
+ @param[in] CharMax The maximum number of Unicode char in String,
+ including terminating null char.
+ @param[out] String String buffer pointer to store the volume label.
+
+ @retval EFI_SUCCESS Volume label is returned.
+ @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
+ @retval EFI_BUFFER_TOO_SMALL The string buffer String cannot hold the
+ volume label.
+
+**/
+EFI_STATUS
+GetVolumeLabel (
+ IN UDF_VOLUME_INFO *Volume,
+ IN UINTN CharMax,
+ OUT CHAR16 *String
+ )
+{
+ UDF_FILE_SET_DESCRIPTOR *FileSetDesc;
+ UINTN Index;
+ UINT8 *OstaCompressed;
+ UINT8 CompressionId;
+ CHAR16 *StringBak;
+
+ FileSetDesc = &Volume->FileSetDesc;
+
+ OstaCompressed = &FileSetDesc->LogicalVolumeIdentifier[0];
+
+ CompressionId = OstaCompressed[0];
+ if (!IS_VALID_COMPRESSION_ID (CompressionId)) {
+ return EFI_VOLUME_CORRUPTED;
+ }
+
+ StringBak = String;
+ for (Index = 1; Index < 128; Index++) {
+ if (CompressionId == 16) {
+ if ((Index >> 1) > CharMax) {
+ return EFI_BUFFER_TOO_SMALL;
+ }
+
+ *String = *(UINT8 *)(OstaCompressed + Index) << 8;
+ Index++;
+ } else {
+ if (Index > CharMax) {
+ return EFI_BUFFER_TOO_SMALL;
+ }
+
+ *String = 0;
+ }
+
+ if (Index < 128) {
+ *String |= (CHAR16)(*(UINT8 *)(OstaCompressed + Index));
+ }
+
+ //
+ // Unlike FID Identifiers, Logical Volume Identifier is stored in a
+ // NULL-terminated OSTA compressed format, so we must check for the NULL
+ // character.
+ //
+ if (*String == L'\0') {
+ break;
+ }
+
+ String++;
+ }
+
+ Index = ((UINTN)String - (UINTN)StringBak) / sizeof (CHAR16);
+ if (Index > CharMax - 1) {
+ Index = CharMax - 1;
+ }
+ StringBak[Index] = L'\0';
+
+ return EFI_SUCCESS;
+}
+
+/**
Get volume and free space size information of an UDF volume.
@attention This is boundary function that may receive untrusted input.
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/Udf.h b/MdeModulePkg/Universal/Disk/UdfDxe/Udf.h
index 9c3f21fd05..7eeaa6192c 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/Udf.h
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/Udf.h
@@ -900,6 +900,33 @@ SetFileInfo (
);
/**
+ Get volume label of an UDF volume.
+
+ @attention This is boundary function that may receive untrusted input.
+ @attention The input is from FileSystem.
+
+ The File Set Descriptor is external input, so this routine will do basic
+ validation for File Set Descriptor and report status.
+
+ @param[in] Volume Volume information pointer.
+ @param[in] CharMax The maximum number of Unicode char in String,
+ including terminating null char.
+ @param[out] String String buffer pointer to store the volume label.
+
+ @retval EFI_SUCCESS Volume label is returned.
+ @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
+ @retval EFI_BUFFER_TOO_SMALL The string buffer String cannot hold the
+ volume label.
+
+**/
+EFI_STATUS
+GetVolumeLabel (
+ IN UDF_VOLUME_INFO *Volume,
+ IN UINTN CharMax,
+ OUT CHAR16 *String
+ );
+
+/**
Get volume and free space size information of an UDF volume.
@attention This is boundary function that may receive untrusted input.
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf b/MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf
index c8bfc880ed..4f435140e9 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf
@@ -58,6 +58,7 @@
[Guids]
gEfiFileInfoGuid ## SOMETIMES_CONSUMES ## Protocol
gEfiFileSystemInfoGuid ## SOMETIMES_CONSUMES ## Protocol
+ gEfiFileSystemVolumeLabelInfoIdGuid ## SOMETIMES_CONSUMES ## Protocol
[Protocols]
--
2.12.0.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v1 09/10] MdeModulePkg/UdfDxe: Add more check when getting PD from LongAd
2018-10-16 7:23 [PATCH v1 00/10] UDF: Bugfixes Hao Wu
` (7 preceding siblings ...)
2018-10-16 7:23 ` [PATCH v1 08/10] MdeModulePkg/UdfDxe: Update GetInfo() for FS VolumeLabel info request Hao Wu
@ 2018-10-16 7:23 ` Hao Wu
2018-10-16 7:23 ` [PATCH v1 10/10] MdeModulePkg/UdfDxe: Avoid possible use of already-freed data Hao Wu
2018-10-22 14:39 ` [PATCH v1 00/10] UDF: Bugfixes Paulo Alcantara
10 siblings, 0 replies; 15+ messages in thread
From: Hao Wu @ 2018-10-16 7:23 UTC (permalink / raw)
To: edk2-devel; +Cc: Hao Wu, Paulo Alcantara, Ruiyu Ni, Star Zeng
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1254
This commit will add an additional check within function GetPdFromLongAd()
when getting a Partition Descriptor from given a Long Allocation
Descriptor.
According to UDF 2.60 Spec, Section 2.2.13:
> The partition reference numbers used are determined by the order of the
> Partition Maps in the LVD.
(Also the picture comes before the above contents)
And a more detailed explanation of the partition reference numbers is at
https://sites.google.com/site/udfintro/ (seems not a formal documentation
though), Section 5.3.6.
Based on the above findings, the 'PartitionReferenceNumber' field in a
Long Allocation Descriptor is used as an index to access the Partition
Maps data within a Logical Volume Descriptor.
Hence, the new check focuses on the validity of this
'PartitionReferenceNumber' field in a Long Allocation Descriptor. Since
the current implementation of UdfDxe driver supports only one partition on
a Logical Volume, so the value of 'PartitionReferenceNumber' should be 0.
Cc: Paulo Alcantara <paulo@paulo.ac>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 562a7d983c..7526de79b2 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -241,11 +241,16 @@ GetPdFromLongAd (
//
// NOTE: Only one Type 1 (Physical) Partition is supported. It has been
// checked already in Partition driver for existence of a single Type 1
- // Partition map, so we don't have to double check here.
+ // Partition map. Hence, the 'PartitionReferenceNumber' field (the index
+ // used to access Partition Maps data within the Logical Volume Descriptor)
+ // in the Long Allocation Descriptor should be 0 to indicate there is only
+ // one partition.
//
- // Partition reference number can also be retrieved from
- // LongAd->ExtentLocation.PartitionReferenceNumber, however the spec says
- // it may be 0, so let's not rely on it.
+ if (LongAd->ExtentLocation.PartitionReferenceNumber != 0) {
+ return NULL;
+ }
+ //
+ // Since only one partition, get the first one directly.
//
PartitionNum = *(UINT16 *)((UINTN)&LogicalVolDesc->PartitionMaps[4]);
break;
--
2.12.0.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v1 10/10] MdeModulePkg/UdfDxe: Avoid possible use of already-freed data
2018-10-16 7:23 [PATCH v1 00/10] UDF: Bugfixes Hao Wu
` (8 preceding siblings ...)
2018-10-16 7:23 ` [PATCH v1 09/10] MdeModulePkg/UdfDxe: Add more check when getting PD from LongAd Hao Wu
@ 2018-10-16 7:23 ` Hao Wu
2018-10-22 14:39 ` [PATCH v1 00/10] UDF: Bugfixes Paulo Alcantara
10 siblings, 0 replies; 15+ messages in thread
From: Hao Wu @ 2018-10-16 7:23 UTC (permalink / raw)
To: edk2-devel; +Cc: Hao Wu, Paulo Alcantara, Ruiyu Ni, Star Zeng
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1255
For function ReadFile():
If the line
Status = GetAedAdsData (
...
);
is reached multiple times during the 'for' loop, freeing the data pointed
by variable 'Data' may potentially lead to variable 'Ad' referencing the
already-freed data.
After calling function GetAllocationDescriptor(), 'Data' and 'Ad' may
point to the same memory (with some possible offset). Hence, this commit
will move the FreePool() call backwards to ensure the data will no longer
be used.
Cc: Paulo Alcantara <paulo@paulo.ac>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 7526de79b2..bf73ab4252 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -1044,6 +1044,7 @@ ReadFile (
EFI_STATUS Status;
UINT32 LogicalBlockSize;
VOID *Data;
+ VOID *DataBak;
UINT64 Length;
VOID *Ad;
UINT64 AdOffset;
@@ -1184,12 +1185,7 @@ ReadFile (
// Descriptor and its extents (ADs).
//
if (GET_EXTENT_FLAGS (RecordingFlags, Ad) == ExtentIsNextExtent) {
- if (!DoFreeAed) {
- DoFreeAed = TRUE;
- } else {
- FreePool (Data);
- }
-
+ DataBak = Data;
Status = GetAedAdsData (
BlockIo,
DiskIo,
@@ -1200,6 +1196,13 @@ ReadFile (
&Data,
&Length
);
+
+ if (!DoFreeAed) {
+ DoFreeAed = TRUE;
+ } else {
+ FreePool (DataBak);
+ }
+
if (EFI_ERROR (Status)) {
goto Error_Get_Aed;
}
--
2.12.0.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v1 00/10] UDF: Bugfixes
2018-10-16 7:23 [PATCH v1 00/10] UDF: Bugfixes Hao Wu
` (9 preceding siblings ...)
2018-10-16 7:23 ` [PATCH v1 10/10] MdeModulePkg/UdfDxe: Avoid possible use of already-freed data Hao Wu
@ 2018-10-22 14:39 ` Paulo Alcantara
2018-10-23 5:45 ` Zeng, Star
10 siblings, 1 reply; 15+ messages in thread
From: Paulo Alcantara @ 2018-10-22 14:39 UTC (permalink / raw)
To: Hao Wu, edk2-devel; +Cc: Hao Wu, Ruiyu Ni, Jiewen Yao, Star Zeng
Hao Wu <hao.a.wu@intel.com> writes:
> The series will address a couple of bugs within the UDF related codes.
>
> Please refer to the log message of each commit for more details.
>
> Cc: Paulo Alcantara <paulo@paulo.ac>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
>
> Hao Wu (10):
> MdeModulePkg/PartitionDxe: Add check for underlying device block size
> MdeModulePkg/UdfDxe: Refine boundary checks for file/path name string
> MdeModulePkg/UdfDxe: Add boundary check the read of FE/EFE
> MdeModulePkg/UdfDxe: Add boundary check for ComponentIdentifier decode
> MdeModulePkg/UdfDxe: Add boundary check for getting volume (free) size
> MdeModulePkg/UdfDxe: Correct behavior for UdfSetPosition()
> MdeModulePkg/UdfDxe: Fix a typo within SetFileInfo()
> MdeModulePkg/UdfDxe: Update GetInfo() for FS VolumeLabel info request
> MdeModulePkg/UdfDxe: Add more check when getting PD from LongAd
> MdeModulePkg/UdfDxe: Avoid possible use of already-freed data
>
> MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 28 +++
> MdeModulePkg/Universal/Disk/UdfDxe/File.c | 96 ++++----
> MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 253 ++++++++++++++++++--
> MdeModulePkg/Universal/Disk/UdfDxe/Udf.h | 63 ++++-
> MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf | 1 +
> 5 files changed, 362 insertions(+), 79 deletions(-)
For the series:
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Thanks!
Paulo
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v1 00/10] UDF: Bugfixes
2018-10-22 14:39 ` [PATCH v1 00/10] UDF: Bugfixes Paulo Alcantara
@ 2018-10-23 5:45 ` Zeng, Star
2018-10-23 6:10 ` Wu, Hao A
0 siblings, 1 reply; 15+ messages in thread
From: Zeng, Star @ 2018-10-23 5:45 UTC (permalink / raw)
To: Paulo Alcantara, Hao Wu, edk2-devel; +Cc: Ruiyu Ni, Jiewen Yao, star.zeng
On 2018/10/22 22:39, Paulo Alcantara wrote:
> Hao Wu <hao.a.wu@intel.com> writes:
>
>> The series will address a couple of bugs within the UDF related codes.
>>
>> Please refer to the log message of each commit for more details.
>>
>> Cc: Paulo Alcantara <paulo@paulo.ac>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>>
>> Hao Wu (10):
>> MdeModulePkg/PartitionDxe: Add check for underlying device block size
>> MdeModulePkg/UdfDxe: Refine boundary checks for file/path name string
>> MdeModulePkg/UdfDxe: Add boundary check the read of FE/EFE
>> MdeModulePkg/UdfDxe: Add boundary check for ComponentIdentifier decode
>> MdeModulePkg/UdfDxe: Add boundary check for getting volume (free) size
>> MdeModulePkg/UdfDxe: Correct behavior for UdfSetPosition()
>> MdeModulePkg/UdfDxe: Fix a typo within SetFileInfo()
>> MdeModulePkg/UdfDxe: Update GetInfo() for FS VolumeLabel info request
>> MdeModulePkg/UdfDxe: Add more check when getting PD from LongAd
>> MdeModulePkg/UdfDxe: Avoid possible use of already-freed data
>>
>> MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 28 +++
>> MdeModulePkg/Universal/Disk/UdfDxe/File.c | 96 ++++----
>> MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 253 ++++++++++++++++++--
>> MdeModulePkg/Universal/Disk/UdfDxe/Udf.h | 63 ++++-
>> MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf | 1 +
>> 5 files changed, 362 insertions(+), 79 deletions(-)
>
> For the series:
>
> Reviewed-by: Paulo Alcantara <palcantara@suse.de>
I could not review the in detail.
Thanks Paulo's Reviewed-by.
I have two minor feedback.
1. I saw some place using 'basic validations' and some place using
'basic validation', should they be aligned?
2. I think you can add "Copyright (c) 2018, Intel Corporation. All
rights reserved.<BR>" for every file you changed.
Acked-by: Star Zeng <star.zeng@intel.com>
Thanks,
Star
>
> Thanks!
> Paulo
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 00/10] UDF: Bugfixes
2018-10-23 5:45 ` Zeng, Star
@ 2018-10-23 6:10 ` Wu, Hao A
2018-10-23 12:28 ` Wu, Hao A
0 siblings, 1 reply; 15+ messages in thread
From: Wu, Hao A @ 2018-10-23 6:10 UTC (permalink / raw)
To: Zeng, Star, Paulo Alcantara, edk2-devel@lists.01.org
Cc: Ni, Ruiyu, Yao, Jiewen
> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, October 23, 2018 1:46 PM
> To: Paulo Alcantara; Wu, Hao A; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu; Yao, Jiewen; Zeng, Star
> Subject: Re: [edk2] [PATCH v1 00/10] UDF: Bugfixes
>
> On 2018/10/22 22:39, Paulo Alcantara wrote:
> > Hao Wu <hao.a.wu@intel.com> writes:
> >
> >> The series will address a couple of bugs within the UDF related codes.
> >>
> >> Please refer to the log message of each commit for more details.
> >>
> >> Cc: Paulo Alcantara <paulo@paulo.ac>
> >> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >> Cc: Star Zeng <star.zeng@intel.com>
> >>
> >> Hao Wu (10):
> >> MdeModulePkg/PartitionDxe: Add check for underlying device block size
> >> MdeModulePkg/UdfDxe: Refine boundary checks for file/path name
> string
> >> MdeModulePkg/UdfDxe: Add boundary check the read of FE/EFE
> >> MdeModulePkg/UdfDxe: Add boundary check for ComponentIdentifier
> decode
> >> MdeModulePkg/UdfDxe: Add boundary check for getting volume (free)
> size
> >> MdeModulePkg/UdfDxe: Correct behavior for UdfSetPosition()
> >> MdeModulePkg/UdfDxe: Fix a typo within SetFileInfo()
> >> MdeModulePkg/UdfDxe: Update GetInfo() for FS VolumeLabel info
> request
> >> MdeModulePkg/UdfDxe: Add more check when getting PD from LongAd
> >> MdeModulePkg/UdfDxe: Avoid possible use of already-freed data
> >>
> >> MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 28 +++
> >> MdeModulePkg/Universal/Disk/UdfDxe/File.c | 96 ++++----
> >> MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 253
> ++++++++++++++++++--
> >> MdeModulePkg/Universal/Disk/UdfDxe/Udf.h | 63 ++++-
> >> MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf | 1 +
> >> 5 files changed, 362 insertions(+), 79 deletions(-)
> >
> > For the series:
> >
> > Reviewed-by: Paulo Alcantara <palcantara@suse.de>
>
> I could not review the in detail.
> Thanks Paulo's Reviewed-by.
>
> I have two minor feedback.
> 1. I saw some place using 'basic validations' and some place using
> 'basic validation', should they be aligned?
> 2. I think you can add "Copyright (c) 2018, Intel Corporation. All
> rights reserved.<BR>" for every file you changed.
>
> Acked-by: Star Zeng <star.zeng@intel.com>
Thanks. I will address the above 2 points before push.
Best Regards,
Hao Wu
>
> Thanks,
> Star
>
> >
> > Thanks!
> > Paulo
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 00/10] UDF: Bugfixes
2018-10-23 6:10 ` Wu, Hao A
@ 2018-10-23 12:28 ` Wu, Hao A
0 siblings, 0 replies; 15+ messages in thread
From: Wu, Hao A @ 2018-10-23 12:28 UTC (permalink / raw)
To: Zeng, Star, Paulo Alcantara, edk2-devel@lists.01.org
Cc: Ni, Ruiyu, Yao, Jiewen
Paulo and Star,
Thanks a lot for the review.
Series pushed at 4df8f5bfa2.. 68099b52b0.
Best Regards,
Hao Wu
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wu,
> Hao A
> Sent: Tuesday, October 23, 2018 2:10 PM
> To: Zeng, Star; Paulo Alcantara; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu; Yao, Jiewen
> Subject: Re: [edk2] [PATCH v1 00/10] UDF: Bugfixes
>
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Tuesday, October 23, 2018 1:46 PM
> > To: Paulo Alcantara; Wu, Hao A; edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu; Yao, Jiewen; Zeng, Star
> > Subject: Re: [edk2] [PATCH v1 00/10] UDF: Bugfixes
> >
> > On 2018/10/22 22:39, Paulo Alcantara wrote:
> > > Hao Wu <hao.a.wu@intel.com> writes:
> > >
> > >> The series will address a couple of bugs within the UDF related codes.
> > >>
> > >> Please refer to the log message of each commit for more details.
> > >>
> > >> Cc: Paulo Alcantara <paulo@paulo.ac>
> > >> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > >> Cc: Jiewen Yao <jiewen.yao@intel.com>
> > >> Cc: Star Zeng <star.zeng@intel.com>
> > >>
> > >> Hao Wu (10):
> > >> MdeModulePkg/PartitionDxe: Add check for underlying device block
> size
> > >> MdeModulePkg/UdfDxe: Refine boundary checks for file/path name
> > string
> > >> MdeModulePkg/UdfDxe: Add boundary check the read of FE/EFE
> > >> MdeModulePkg/UdfDxe: Add boundary check for ComponentIdentifier
> > decode
> > >> MdeModulePkg/UdfDxe: Add boundary check for getting volume (free)
> > size
> > >> MdeModulePkg/UdfDxe: Correct behavior for UdfSetPosition()
> > >> MdeModulePkg/UdfDxe: Fix a typo within SetFileInfo()
> > >> MdeModulePkg/UdfDxe: Update GetInfo() for FS VolumeLabel info
> > request
> > >> MdeModulePkg/UdfDxe: Add more check when getting PD from LongAd
> > >> MdeModulePkg/UdfDxe: Avoid possible use of already-freed data
> > >>
> > >> MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 28 +++
> > >> MdeModulePkg/Universal/Disk/UdfDxe/File.c | 96 ++++----
> > >> MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 253
> > ++++++++++++++++++--
> > >> MdeModulePkg/Universal/Disk/UdfDxe/Udf.h | 63 ++++-
> > >> MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf | 1 +
> > >> 5 files changed, 362 insertions(+), 79 deletions(-)
> > >
> > > For the series:
> > >
> > > Reviewed-by: Paulo Alcantara <palcantara@suse.de>
> >
> > I could not review the in detail.
> > Thanks Paulo's Reviewed-by.
> >
> > I have two minor feedback.
> > 1. I saw some place using 'basic validations' and some place using
> > 'basic validation', should they be aligned?
> > 2. I think you can add "Copyright (c) 2018, Intel Corporation. All
> > rights reserved.<BR>" for every file you changed.
> >
> > Acked-by: Star Zeng <star.zeng@intel.com>
>
> Thanks. I will address the above 2 points before push.
>
> Best Regards,
> Hao Wu
>
> >
> > Thanks,
> > Star
> >
> > >
> > > Thanks!
> > > Paulo
> > >
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 15+ messages in thread