[PATCH v1 00/10] UDF: Bugfixes

[PATCH v1 00/10] UDF: Bugfixes

[Hao Wu]

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(-)

-- 
2.12.0.windows.1

[PATCH v1 01/10] MdeModulePkg/PartitionDxe: Add check for underlying device block size

[Hao Wu]

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

[PATCH v1 02/10] MdeModulePkg/UdfDxe: Refine boundary checks for file/path name string

[Hao Wu]

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

[PATCH v1 03/10] MdeModulePkg/UdfDxe: Add boundary check the read of FE/EFE

[Hao Wu]

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

[PATCH v1 04/10] MdeModulePkg/UdfDxe: Add boundary check for ComponentIdentifier decode

[Hao Wu]

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

[PATCH v1 05/10] MdeModulePkg/UdfDxe: Add boundary check for getting volume (free) size

[Hao Wu]

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

[PATCH v1 06/10] MdeModulePkg/UdfDxe: Correct behavior for UdfSetPosition()

[Hao Wu]

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

[PATCH v1 07/10] MdeModulePkg/UdfDxe: Fix a typo within SetFileInfo()

[Hao Wu]

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

[PATCH v1 08/10] MdeModulePkg/UdfDxe: Update GetInfo() for FS VolumeLabel info request

[Hao Wu]

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

[PATCH v1 09/10] MdeModulePkg/UdfDxe: Add more check when getting PD from LongAd

[Hao Wu]

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

[PATCH v1 10/10] MdeModulePkg/UdfDxe: Avoid possible use of already-freed data

[Hao Wu]

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

Re: [PATCH v1 00/10] UDF: Bugfixes

[Paulo Alcantara]

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

Re: [PATCH v1 00/10] UDF: Bugfixes

[Zeng, Star]

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
> 

Re: [PATCH v1 00/10] UDF: Bugfixes

[Wu, Hao A]

> -----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
> >

Re: [PATCH v1 00/10] UDF: Bugfixes

[Wu, Hao A]

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