[PATCH 0/3] UDF partition driver fix

[PATCH 0/3] UDF partition driver fix

[Paulo Alcantara]

This series fixes an UDF issue with Partition driver as discussed in ML:

https://lists.01.org/pipermail/edk2-devel/2017-September/014694.html

Thanks!
Paulo

Repo:   https://github.com/pcacjr/edk2.git
Branch: udf-partition-fix

Paulo Alcantara (3):
  MdePkg: Add UDF volume structure definitions
  MdeModulePkg/PartitionDxe: Fix creation of UDF logical partition
  MdeModulePkg/UdfDxe: Rework driver to support PartitionDxe changes

 MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c     | 307 +++++++++++-
 MdeModulePkg/Universal/Disk/UdfDxe/File.c          |  13 +-
 .../Universal/Disk/UdfDxe/FileSystemOperations.c   | 525 ++++++++-------------
 MdeModulePkg/Universal/Disk/UdfDxe/Udf.c           |   7 -
 MdeModulePkg/Universal/Disk/UdfDxe/Udf.h           |  88 +---
 MdePkg/Include/IndustryStandard/Udf.h              |  63 +++
 6 files changed, 560 insertions(+), 443 deletions(-)

-- 
2.11.0

[PATCH 1/3] MdePkg: Add UDF volume structure definitions

[Paulo Alcantara]

This patch adds a fewe more UDF structures in order to detect Logical
Volume and Partition descriptors during Main Volume Descriptor Sequence
in Partition driver.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
---
 MdePkg/Include/IndustryStandard/Udf.h | 63 +++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/MdePkg/Include/IndustryStandard/Udf.h b/MdePkg/Include/IndustryStandard/Udf.h
index 0febb4bcda..6cc42f8543 100644
--- a/MdePkg/Include/IndustryStandard/Udf.h
+++ b/MdePkg/Include/IndustryStandard/Udf.h
@@ -27,9 +27,19 @@
 #define _GET_TAG_ID(_Pointer) \
   (((UDF_DESCRIPTOR_TAG *)(_Pointer))->TagIdentifier)
 
+#define IS_PD(_Pointer) \
+  ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 5))
+#define IS_LVD(_Pointer) \
+  ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 6))
+#define IS_TD(_Pointer) \
+  ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 8))
+
 #define IS_AVDP(_Pointer) \
   ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 2))
 
+#define LV_UDF_REVISION(_Lv) \
+  *(UINT16 *)(UINTN)(_Lv)->DomainIdentifier.IdentifierSuffix
+
 #pragma pack(1)
 
 typedef struct {
@@ -55,6 +65,59 @@ typedef struct {
   UINT8               Reserved[480];
 } UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER;
 
+typedef struct {
+  UINT8           CharacterSetType;
+  UINT8           CharacterSetInfo[63];
+} UDF_CHAR_SPEC;
+
+typedef struct {
+  UINT8           Flags;
+  UINT8           Identifier[23];
+  UINT8           IdentifierSuffix[8];
+} UDF_ENTITY_ID;
+
+typedef struct {
+  UINT32        LogicalBlockNumber;
+  UINT16        PartitionReferenceNumber;
+} UDF_LB_ADDR;
+
+typedef struct {
+  UINT32                           ExtentLength;
+  UDF_LB_ADDR                      ExtentLocation;
+  UINT8                            ImplementationUse[6];
+} UDF_LONG_ALLOCATION_DESCRIPTOR;
+
+typedef struct {
+  UDF_DESCRIPTOR_TAG              DescriptorTag;
+  UINT32                          VolumeDescriptorSequenceNumber;
+  UDF_CHAR_SPEC                   DescriptorCharacterSet;
+  UINT8                           LogicalVolumeIdentifier[128];
+  UINT32                          LogicalBlockSize;
+  UDF_ENTITY_ID                   DomainIdentifier;
+  UDF_LONG_ALLOCATION_DESCRIPTOR  LogicalVolumeContentsUse;
+  UINT32                          MapTableLength;
+  UINT32                          NumberOfPartitionMaps;
+  UDF_ENTITY_ID                   ImplementationIdentifier;
+  UINT8                           ImplementationUse[128];
+  UDF_EXTENT_AD                   IntegritySequenceExtent;
+  UINT8                           PartitionMaps[6];
+} UDF_LOGICAL_VOLUME_DESCRIPTOR;
+
+typedef struct {
+  UDF_DESCRIPTOR_TAG         DescriptorTag;
+  UINT32                     VolumeDescriptorSequenceNumber;
+  UINT16                     PartitionFlags;
+  UINT16                     PartitionNumber;
+  UDF_ENTITY_ID              PartitionContents;
+  UINT8                      PartitionContentsUse[128];
+  UINT32                     AccessType;
+  UINT32                     PartitionStartingLocation;
+  UINT32                     PartitionLength;
+  UDF_ENTITY_ID              ImplementationIdentifier;
+  UINT8                      ImplementationUse[128];
+  UINT8                      Reserved[156];
+} UDF_PARTITION_DESCRIPTOR;
+
 #pragma pack()
 
 #endif
-- 
2.11.0

[PATCH 2/3] MdeModulePkg/PartitionDxe: Fix creation of UDF logical partition

[Paulo Alcantara]

Do not use entire block device size for the UDF logical partition,
instead reserve the appropriate space (LVD space) for it.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Reported-by: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
---
 MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 307 +++++++++++++++++++++++--
 1 file changed, 283 insertions(+), 24 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
index 7856b5dfc1..b00369ba6d 100644
--- a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
+++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
@@ -84,24 +84,8 @@ FindAnchorVolumeDescriptorPointer (
   return EFI_VOLUME_CORRUPTED;
 }
 
-/**
-  Check if block device supports a valid UDF file system as specified by OSTA
-  Universal Disk Format Specification 2.60.
-
-  @param[in]   BlockIo  BlockIo interface.
-  @param[in]   DiskIo   DiskIo interface.
-
-  @retval EFI_SUCCESS          UDF file system found.
-  @retval EFI_UNSUPPORTED      UDF file system not found.
-  @retval EFI_NO_MEDIA         The device has no media.
-  @retval EFI_DEVICE_ERROR     The device reported an error.
-  @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
-  @retval EFI_OUT_OF_RESOURCES The scan was not successful due to lack of
-                               resources.
-
-**/
 EFI_STATUS
-SupportUdfFileSystem (
+FindUdfVolumeIdentifiers (
   IN EFI_BLOCK_IO_PROTOCOL  *BlockIo,
   IN EFI_DISK_IO_PROTOCOL   *DiskIo
   )
@@ -111,7 +95,6 @@ SupportUdfFileSystem (
   UINT64                                EndDiskOffset;
   CDROM_VOLUME_DESCRIPTOR               VolDescriptor;
   CDROM_VOLUME_DESCRIPTOR               TerminatingVolDescriptor;
-  UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER  AnchorPoint;
 
   ZeroMem ((VOID *)&TerminatingVolDescriptor, sizeof (CDROM_VOLUME_DESCRIPTOR));
 
@@ -207,12 +190,286 @@ SupportUdfFileSystem (
     return EFI_UNSUPPORTED;
   }
 
+  return EFI_SUCCESS;
+}
+
+EFI_STATUS
+GetPartitionNumber (
+  IN   UDF_LOGICAL_VOLUME_DESCRIPTOR  *LogicalVolDesc,
+  OUT  UINT16                         *PartitionNum
+  )
+{
+  EFI_STATUS                      Status;
+  UDF_LONG_ALLOCATION_DESCRIPTOR  *LongAd;
+
+  Status = EFI_SUCCESS;
+
+  switch (LV_UDF_REVISION (LogicalVolDesc)) {
+  case 0x0102:
+    //
+    // UDF 1.20 only supports Type 1 Partition
+    //
+    *PartitionNum = *(UINT16 *)((UINTN)&LogicalVolDesc->PartitionMaps[4]);
+    break;
+  case 0x0150:
+    //
+    // Ensure Type 1 Partition map
+    //
+    ASSERT (LogicalVolDesc->PartitionMaps[0] == 1 &&
+            LogicalVolDesc->PartitionMaps[1] == 6);
+    *PartitionNum = *(UINT16 *)((UINTN)&LogicalVolDesc->PartitionMaps[4]);
+    break;
+  case 0x0260:
+    LongAd = &LogicalVolDesc->LogicalVolumeContentsUse;
+    *PartitionNum = LongAd->ExtentLocation.PartitionReferenceNumber;
+    break;
+  default:
+    //
+    // Unhandled UDF revision
+    //
+    Status = EFI_VOLUME_CORRUPTED;
+    break;
+  }
+
+  return Status;
+}
+
+EFI_STATUS
+FindLogicalVolumeLocation (
+  IN   EFI_BLOCK_IO_PROTOCOL                 *BlockIo,
+  IN   EFI_DISK_IO_PROTOCOL                  *DiskIo,
+  IN   UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER  *AnchorPoint,
+  OUT  UINT64                                *MainVdsStartLsn,
+  OUT  UINT64                                *LogicalVolEndLsn
+  )
+{
+  EFI_STATUS                     Status;
+  UINT32                         BlockSize;
+  EFI_LBA                        LastBlock;
+  UDF_EXTENT_AD                  *ExtentAd;
+  UINT64                         StartingLsn;
+  UINT64                         EndingLsn;
+  VOID                           *Buffer;
+  UDF_LOGICAL_VOLUME_DESCRIPTOR  *LogicalVolDesc;
+  UDF_PARTITION_DESCRIPTOR       *PartitionDesc;
+  UINT64                         GuardMainVdsStartLsn;
+  UINT16                         PartitionNum;
+
+  BlockSize             = BlockIo->Media->BlockSize;
+  LastBlock             = BlockIo->Media->LastBlock;
+  ExtentAd              = &AnchorPoint->MainVolumeDescriptorSequenceExtent;
+  StartingLsn           = (UINT64)ExtentAd->ExtentLocation;
+  EndingLsn             =
+    StartingLsn + DivU64x32 ((UINT64)ExtentAd->ExtentLength, BlockSize);
+
+  LogicalVolDesc        = NULL;
+  PartitionDesc         = NULL;
+  GuardMainVdsStartLsn  = StartingLsn;
+
+  //
+  // Allocate buffer for reading disk blocks
+  //
+  Buffer = AllocateZeroPool (BlockSize);
+  if (Buffer == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  Status = EFI_VOLUME_CORRUPTED;
+
+  //
+  // As per UDF 2.60 specification:
+  //
+  // There shall be exactly one prevailing Logical Volume Descriptor
+  // recorded per Volume Set.
+  //
+  // Start Main Volume Descriptor Sequence and find Logical Volume Descriptor
+  //
+  while (StartingLsn <= EndingLsn) {
+    //
+    // Read disk block
+    //
+    Status = DiskIo->ReadDisk (
+      DiskIo,
+      BlockIo->Media->MediaId,
+      MultU64x32 (StartingLsn, BlockSize),
+      BlockSize,
+      Buffer
+      );
+    if (EFI_ERROR (Status)) {
+      goto Out_Free;
+    }
+
+    //
+    // Check if read block is a Terminating Descriptor
+    //
+    if (IS_TD (Buffer)) {
+      //
+      // Stop Main Volume Descriptor Sequence
+      //
+      break;
+    }
+
+    //
+    // Check if read block is a Logical Volume Descriptor
+    //
+    if (IS_LVD (Buffer)) {
+      //
+      // Ensure only one LVD (Logical Volume Descriptor) is read
+      //
+      ASSERT (LogicalVolDesc == NULL);
+
+      //
+      // As per UDF 2.60 specification:
+      //
+      // For the purpose of interchange, Partition Maps shall be limited to
+      // Partition Map type 1, except type 2 maps.
+      //
+      // NOTE: Type 1 Partitions are the only supported in this implementation.
+      //
+      LogicalVolDesc = AllocateZeroPool (sizeof (*LogicalVolDesc));
+      if (LogicalVolDesc == NULL) {
+        Status = EFI_OUT_OF_RESOURCES;
+        goto Out_Free;
+      }
+
+      //
+      // Save Logical Volume Descriptor
+      //
+      CopyMem (LogicalVolDesc, Buffer, sizeof (*LogicalVolDesc));
+    } else if (IS_PD (Buffer)) {
+      //
+      // Ensure only one PD (Partition Descriptor) is read
+      //
+      ASSERT (PartitionDesc == NULL);
+      //
+      // Found a Partition Descriptor.
+      //
+      // As per UDF 2.60 specification:
+      //
+      // A Partition Descriptor Access Type of read-only, rewritable,
+      // overwritable, write-once and pseudo-overwritable shall be
+      // supported. There shall be exactly one prevailing Partition
+      // Descriptor recorded per volume, with one exception. For Volume
+      // Sets that consist of single volume, the volume may contain 2 non-
+      // overlapping Partitions with 2 prevailing Partition Descriptors only
+      // if one has an Access Type of read-only and the other has an
+      // Access Type of rewritable, overwritable, or write-once. The
+      // Logical Volume for this volume would consist of the contents of
+      // both partitions.
+      //
+      PartitionDesc = AllocateZeroPool (sizeof (*PartitionDesc));
+      if (PartitionDesc == NULL) {
+        Status = EFI_OUT_OF_RESOURCES;
+        goto Out_Free;
+      }
+
+      //
+      // Save Partition Descriptor
+      //
+      CopyMem (PartitionDesc, Buffer, sizeof (*PartitionDesc));
+    }
+
+    //
+    // Go to next disk block
+    //
+    StartingLsn++;
+  }
+
+  //
+  // Check if LVD and PD were found
+  //
+  if (LogicalVolDesc != NULL && PartitionDesc != NULL) {
+    //
+    // Get partition number from Partition map in LVD descriptor
+    //
+    Status = GetPartitionNumber (LogicalVolDesc, &PartitionNum);
+    if (EFI_ERROR (Status)) {
+      goto Out_Free;
+    }
+
+    //
+    // Make sure we're handling expected Partition Descriptor
+    //
+    ASSERT (PartitionNum == PartitionDesc->PartitionNumber);
+
+    //
+    // Cover the main VDS area so UdfDxe driver will also be able to get LVD and
+    // PD descriptors out from the file system.
+    //
+    *MainVdsStartLsn = GuardMainVdsStartLsn;
+    *LogicalVolEndLsn = *MainVdsStartLsn + (UINT64)ExtentAd->ExtentLength;
+
+    //
+    // Cover UDF partition area
+    //
+    *LogicalVolEndLsn +=
+      ((UINT64)PartitionDesc->PartitionStartingLocation -
+       *LogicalVolEndLsn) + PartitionDesc->PartitionLength - 1;
+    //
+    // Ensure to not attempt reading past end of device
+    //
+    ASSERT (*LogicalVolEndLsn <= LastBlock);
+
+    Status = EFI_SUCCESS;
+  }
+
+Out_Free:
+  //
+  // Free block read buffer
+  //
+  FreePool (Buffer);
+  //
+  // Free Logical Volume Descriptor
+  //
+  if (LogicalVolDesc != NULL) {
+    FreePool (LogicalVolDesc);
+  }
+  //
+  // Free Partition Descriptor
+  //
+  if (PartitionDesc != NULL) {
+    FreePool (PartitionDesc);
+  }
+
+  return Status;
+}
+
+EFI_STATUS
+FindUdfLogicalVolume (
+  IN EFI_BLOCK_IO_PROTOCOL  *BlockIo,
+  IN EFI_DISK_IO_PROTOCOL   *DiskIo,
+  OUT EFI_LBA               *StartingLBA,
+  OUT EFI_LBA               *EndingLBA
+  )
+{
+  EFI_STATUS Status;
+  UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER  AnchorPoint;
+
+  //
+  // Find UDF volume identifiers
+  //
+  Status = FindUdfVolumeIdentifiers (BlockIo, DiskIo);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  //
+  // Find Anchor Volume Descriptor Pointer
+  //
   Status = FindAnchorVolumeDescriptorPointer (BlockIo, DiskIo, &AnchorPoint);
   if (EFI_ERROR (Status)) {
-    return EFI_UNSUPPORTED;
+    return Status;
   }
 
-  return EFI_SUCCESS;
+  //
+  // Find Logical Volume location
+  //
+  Status = FindLogicalVolumeLocation (
+    BlockIo, DiskIo, &AnchorPoint, (UINT64 *)StartingLBA,
+    (UINT64 *)EndingLBA
+    );
+
+  return Status;
 }
 
 /**
@@ -250,6 +507,8 @@ PartitionInstallUdfChildHandles (
   EFI_GUID                     *VendorDefinedGuid;
   EFI_GUID                     UdfDevPathGuid = EFI_UDF_DEVICE_PATH_GUID;
   EFI_PARTITION_INFO_PROTOCOL  PartitionInfo;
+  EFI_LBA                      StartingLBA;
+  EFI_LBA                      EndingLBA;
 
   Media = BlockIo->Media;
 
@@ -291,9 +550,9 @@ PartitionInstallUdfChildHandles (
   }
 
   //
-  // Check if block device supports an UDF file system
+  // Find UDF logical volume on block device
   //
-  Status = SupportUdfFileSystem (BlockIo, DiskIo);
+  Status = FindUdfLogicalVolume (BlockIo, DiskIo, &StartingLBA, &EndingLBA);
   if (EFI_ERROR (Status)) {
     return EFI_NOT_FOUND;
   }
@@ -318,8 +577,8 @@ PartitionInstallUdfChildHandles (
     DevicePath,
     (EFI_DEVICE_PATH_PROTOCOL *)&gUdfDevicePath,
     &PartitionInfo,
-    0,
-    Media->LastBlock,
+    StartingLBA,
+    EndingLBA,
     Media->BlockSize
     );
   if (!EFI_ERROR (Status)) {
-- 
2.11.0

[PATCH 3/3] MdeModulePkg/UdfDxe: Rework driver to support PartitionDxe changes

[Paulo Alcantara]

This patch reworks the driver to support Partition driver changes.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Paulo Alcantara <pcacjr@zytor.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
---
 MdeModulePkg/Universal/Disk/UdfDxe/File.c          |  13 +-
 .../Universal/Disk/UdfDxe/FileSystemOperations.c   | 525 ++++++++-------------
 MdeModulePkg/Universal/Disk/UdfDxe/Udf.c           |   7 -
 MdeModulePkg/Universal/Disk/UdfDxe/Udf.h           |  88 +---
 4 files changed, 214 insertions(+), 419 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
index 01361141bb..f2c62967e8 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
@@ -100,6 +100,7 @@ UdfOpenVolume (
     &PrivFsData->Volume,
     &PrivFsData->Root
     );
+  ASSERT_EFI_ERROR (Status);
   if (EFI_ERROR (Status)) {
     goto Error_Find_Root_Dir;
   }
@@ -131,7 +132,6 @@ Error_Alloc_Priv_File_Data:
   CleanupFileInformation (&PrivFsData->Root);
 
 Error_Find_Root_Dir:
-  CleanupVolumeInformation (&PrivFsData->Volume);
 
 Error_Read_Udf_Volume:
 Error_Invalid_Params:
@@ -528,7 +528,6 @@ UdfClose (
   EFI_TPL                     OldTpl;
   EFI_STATUS                  Status;
   PRIVATE_UDF_FILE_DATA       *PrivFileData;
-  PRIVATE_UDF_SIMPLE_FS_DATA  *PrivFsData;
 
   OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
 
@@ -541,8 +540,6 @@ UdfClose (
 
   PrivFileData = PRIVATE_UDF_FILE_DATA_FROM_THIS (This);
 
-  PrivFsData = PRIVATE_UDF_SIMPLE_FS_DATA_FROM_THIS (PrivFileData->SimpleFs);
-
   if (!PrivFileData->IsRootDirectory) {
     CleanupFileInformation (&PrivFileData->File);
 
@@ -551,10 +548,6 @@ UdfClose (
     }
   }
 
-  if (--PrivFsData->OpenFiles == 0) {
-    CleanupVolumeInformation (&PrivFsData->Volume);
-  }
-
   FreePool ((VOID *)PrivFileData);
 
 Exit:
@@ -787,7 +780,7 @@ UdfGetInfo (
   } else if (CompareGuid (InformationType, &gEfiFileSystemInfoGuid)) {
     String = VolumeLabel;
 
-    FileSetDesc = PrivFsData->Volume.FileSetDescs[0];
+    FileSetDesc = &PrivFsData->Volume.FileSetDesc;
 
     OstaCompressed = &FileSetDesc->LogicalVolumeIdentifier[0];
 
@@ -846,7 +839,7 @@ UdfGetInfo (
     FileSystemInfo->Size        = FileSystemInfoLength;
     FileSystemInfo->ReadOnly    = TRUE;
     FileSystemInfo->BlockSize   =
-      LV_BLOCK_SIZE (&PrivFsData->Volume, UDF_DEFAULT_LV_NUM);
+      PrivFsData->Volume.LogicalVolDesc.LogicalBlockSize;
     FileSystemInfo->VolumeSize  = VolumeSize;
     FileSystemInfo->FreeSpace   = FreeSpaceSize;
 
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 4609580b30..0f4bc02861 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -60,154 +60,111 @@ FindAnchorVolumeDescriptorPointer (
 
 EFI_STATUS
 StartMainVolumeDescriptorSequence (
-  IN   EFI_BLOCK_IO_PROTOCOL                 *BlockIo,
-  IN   EFI_DISK_IO_PROTOCOL                  *DiskIo,
-  IN   UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER  *AnchorPoint,
-  OUT  UDF_VOLUME_INFO                       *Volume
+  IN   EFI_BLOCK_IO_PROTOCOL    *BlockIo,
+  IN   EFI_DISK_IO_PROTOCOL     *DiskIo,
+  OUT  UDF_VOLUME_INFO          *Volume
   )
 {
-  EFI_STATUS                     Status;
-  UINT32                         BlockSize;
-  UDF_EXTENT_AD                  *ExtentAd;
-  UINT64                         StartingLsn;
-  UINT64                         EndingLsn;
-  VOID                           *Buffer;
-  UDF_LOGICAL_VOLUME_DESCRIPTOR  *LogicalVolDesc;
-  UDF_PARTITION_DESCRIPTOR       *PartitionDesc;
-  UINTN                          Index;
-  UINT32                         LogicalBlockSize;
+  EFI_STATUS  Status;
+  UINT32      BlockSize;
+  UINT64      BlockOffset;
+  VOID        *Buffer;
+  UINT32      LogicalBlockSize;
+
+  BlockSize = BlockIo->Media->BlockSize;
 
   //
-  // We've already found an ADVP on the volume. It contains the extent
-  // (MainVolumeDescriptorSequenceExtent) where the Main Volume Descriptor
-  // Sequence starts. Therefore, we'll look for Logical Volume Descriptors and
-  // Partitions Descriptors and save them in memory, accordingly.
-  //
-  // Note also that each descriptor will be aligned on a block size (BlockSize)
-  // boundary, so we need to read one block at a time.
+  // Allocate buffer for reading disk blocks
   //
-  BlockSize    = BlockIo->Media->BlockSize;
-  ExtentAd     = &AnchorPoint->MainVolumeDescriptorSequenceExtent;
-  StartingLsn  = (UINT64)ExtentAd->ExtentLocation;
-  EndingLsn    = StartingLsn + DivU64x32 (
-                                     (UINT64)ExtentAd->ExtentLength,
-                                     BlockSize
-                                     );
-
-  Volume->LogicalVolDescs =
-    (UDF_LOGICAL_VOLUME_DESCRIPTOR **)AllocateZeroPool (ExtentAd->ExtentLength);
-  if (Volume->LogicalVolDescs == NULL) {
-    return EFI_OUT_OF_RESOURCES;
-  }
-
-  Volume->PartitionDescs =
-    (UDF_PARTITION_DESCRIPTOR **)AllocateZeroPool (ExtentAd->ExtentLength);
-  if (Volume->PartitionDescs == NULL) {
-    Status = EFI_OUT_OF_RESOURCES;
-    goto Error_Alloc_Pds;
-  }
-
-  Buffer = AllocateZeroPool (BlockSize);
+  Buffer = AllocateZeroPool ((UINTN)BlockSize);
   if (Buffer == NULL) {
-    Status = EFI_OUT_OF_RESOURCES;
-    goto Error_Alloc_Buf;
+    return EFI_OUT_OF_RESOURCES;
   }
 
-  Volume->LogicalVolDescsNo  = 0;
-  Volume->PartitionDescsNo   = 0;
-
-  while (StartingLsn <= EndingLsn) {
+  //
+  // As per UDF 2.60 specification:
+  //
+  // ---
+  // For the purpose of interchange, Partition Maps shall be limited to
+  // Partition Map type 1, except type 2 maps.
+  // ---
+  // A Partition Descriptor Access Type of read-only, rewritable,
+  // overwritable, write-once and pseudo-overwritable shall be
+  // supported. There shall be exactly one prevailing Partition
+  // Descriptor recorded per volume, with one exception. For Volume
+  // Sets that consist of single volume, the volume may contain 2 non-
+  // overlapping Partitions with 2 prevailing Partition Descriptors only
+  // if one has an Access Type of read-only and the other has an
+  // Access Type of rewritable, overwritable, or write-once. The
+  // Logical Volume for this volume would consist of the contents of
+  // both partitions.
+  // ---
+  //
+  // The logical partition created by Partition driver is relative to the main
+  // VDS extent location, so we start the Main Volume Descriptor Sequence at
+  // LBA 0.
+  //
+  // We don't need to make sure we got both LVD and PD -- that is, if we are
+  // here, that means Partition driver was able to find them both previously.
+  //
+  for (BlockOffset = 0;
+       BlockOffset < MultU64x32 (BlockIo->Media->LastBlock,
+                                 BlockIo->Media->BlockSize);
+       BlockOffset += BlockSize) {
+    // Read disk block
+    //
     Status = DiskIo->ReadDisk (
       DiskIo,
       BlockIo->Media->MediaId,
-      MultU64x32 (StartingLsn, BlockSize),
+      BlockOffset,
       BlockSize,
       Buffer
       );
     if (EFI_ERROR (Status)) {
-      goto Error_Read_Disk_Blk;
+      goto Out_Free;
     }
 
+    //
+    // Check if read block is a Terminating Descriptor
+    //
     if (IS_TD (Buffer)) {
       //
-      // Found a Terminating Descriptor. Stop the sequence then.
+      // Terminate Main Volume Descriptor Sequence
       //
       break;
     }
 
     if (IS_LVD (Buffer)) {
       //
-      // Found a Logical Volume Descriptor.
+      // Save Logical Volume Descriptor
       //
-      LogicalVolDesc =
-        (UDF_LOGICAL_VOLUME_DESCRIPTOR *)
-        AllocateZeroPool (sizeof (UDF_LOGICAL_VOLUME_DESCRIPTOR));
-      if (LogicalVolDesc == NULL) {
-        Status = EFI_OUT_OF_RESOURCES;
-        goto Error_Alloc_Lvd;
-      }
-
-      CopyMem ((VOID *)LogicalVolDesc, Buffer,
-               sizeof (UDF_LOGICAL_VOLUME_DESCRIPTOR));
-      Volume->LogicalVolDescs[Volume->LogicalVolDescsNo++] = LogicalVolDesc;
+      CopyMem (&Volume->LogicalVolDesc, Buffer, sizeof (Volume->LogicalVolDesc));
     } else if (IS_PD (Buffer)) {
       //
-      // Found a Partition Descriptor.
+      // Save Partition Descriptor
       //
-      PartitionDesc =
-        (UDF_PARTITION_DESCRIPTOR *)
-        AllocateZeroPool (sizeof (UDF_PARTITION_DESCRIPTOR));
-      if (PartitionDesc == NULL) {
-        Status = EFI_OUT_OF_RESOURCES;
-        goto Error_Alloc_Pd;
-      }
-
-      CopyMem ((VOID *)PartitionDesc, Buffer,
-               sizeof (UDF_PARTITION_DESCRIPTOR));
-      Volume->PartitionDescs[Volume->PartitionDescsNo++] = PartitionDesc;
+      CopyMem (&Volume->PartitionDesc, Buffer, sizeof (Volume->PartitionDesc));
     }
-
-    StartingLsn++;
   }
 
   //
-  // When an UDF volume (revision 2.00 or higher) contains a File Entry rather
-  // than an Extended File Entry (which is not recommended as per spec), we need
-  // to make sure the size of a FE will be _at least_ 2048
-  // (UDF_LOGICAL_SECTOR_SIZE) bytes long to keep backward compatibility.
+  // Determine FE (File Entry) size
   //
-  LogicalBlockSize = LV_BLOCK_SIZE (Volume, UDF_DEFAULT_LV_NUM);
+  LogicalBlockSize = Volume->LogicalVolDesc.LogicalBlockSize;
   if (LogicalBlockSize >= UDF_LOGICAL_SECTOR_SIZE) {
-    Volume->FileEntrySize = LogicalBlockSize;
+    Volume->FileEntrySize = (UINTN)LogicalBlockSize;
   } else {
     Volume->FileEntrySize = UDF_LOGICAL_SECTOR_SIZE;
   }
 
-  FreePool (Buffer);
-
-  return EFI_SUCCESS;
-
-Error_Alloc_Pd:
-Error_Alloc_Lvd:
-  for (Index = 0; Index < Volume->PartitionDescsNo; Index++) {
-    FreePool ((VOID *)Volume->PartitionDescs[Index]);
-  }
-
-  for (Index = 0; Index < Volume->LogicalVolDescsNo; Index++) {
-    FreePool ((VOID *)Volume->LogicalVolDescs[Index]);
-  }
+  Status = EFI_SUCCESS;
 
-Error_Read_Disk_Blk:
+Out_Free:
+  //
+  // Free block read buffer
+  //
   FreePool (Buffer);
 
-Error_Alloc_Buf:
-  FreePool ((VOID *)Volume->PartitionDescs);
-  Volume->PartitionDescs = NULL;
-
-Error_Alloc_Pds:
-  FreePool ((VOID *)Volume->LogicalVolDescs);
-  Volume->LogicalVolDescs = NULL;
-
   return Status;
 }
 
@@ -223,11 +180,11 @@ GetPdFromLongAd (
   )
 {
   UDF_LOGICAL_VOLUME_DESCRIPTOR  *LogicalVolDesc;
-  UINTN                          Index;
-  UDF_PARTITION_DESCRIPTOR       *PartitionDesc;
   UINT16                         PartitionNum;
 
-  LogicalVolDesc = Volume->LogicalVolDescs[UDF_DEFAULT_LV_NUM];
+  LogicalVolDesc = &Volume->LogicalVolDesc;
+
+  DEBUG ((DEBUG_ERROR, "rev 0x%04x\n", LV_UDF_REVISION (LogicalVolDesc)));
 
   switch (LV_UDF_REVISION (LogicalVolDesc)) {
   case 0x0102:
@@ -252,19 +209,25 @@ GetPdFromLongAd (
     PartitionNum = *(UINT16 *)((UINTN)&LogicalVolDesc->PartitionMaps[4]);
     break;
   case 0x0260:
-    //
-    // Fall through.
-    //
-  default:
     PartitionNum = LongAd->ExtentLocation.PartitionReferenceNumber;
     break;
+  default:
+    //
+    // Unhandled UDF revision
+    //
+    return NULL;
   }
 
-  for (Index = 0; Index < Volume->PartitionDescsNo; Index++) {
-    PartitionDesc = Volume->PartitionDescs[Index];
-    if (PartitionDesc->PartitionNumber == PartitionNum) {
-      return PartitionDesc;
-    }
+  //
+  // Check if partition number matches Partition Descriptor found in Main Volume
+  // Descriptor Sequence.
+  //
+  DEBUG ((DEBUG_ERROR, "PartitionNumber %d\n",
+          Volume->PartitionDesc.PartitionNumber));
+  DEBUG ((DEBUG_ERROR, "PartitionNumber %d\n",
+          PartitionNum));
+  if (Volume->PartitionDesc.PartitionNumber == PartitionNum) {
+    return &Volume->PartitionDesc;
   }
 
   return NULL;
@@ -284,8 +247,9 @@ GetLongAdLsn (
   PartitionDesc = GetPdFromLongAd (Volume, LongAd);
   ASSERT (PartitionDesc != NULL);
 
-  return (UINT64)PartitionDesc->PartitionStartingLocation +
-                 LongAd->ExtentLocation.LogicalBlockNumber;
+  return (UINT64)PartitionDesc->PartitionStartingLocation -
+    Volume->MainVdsStartLocation +
+    LongAd->ExtentLocation.LogicalBlockNumber;
 }
 
 //
@@ -293,12 +257,13 @@ GetLongAdLsn (
 //
 UINT64
 GetShortAdLsn (
+  IN UDF_VOLUME_INFO                  *Volume,
   IN UDF_PARTITION_DESCRIPTOR         *PartitionDesc,
   IN UDF_SHORT_ALLOCATION_DESCRIPTOR  *ShortAd
   )
 {
-  return (UINT64)PartitionDesc->PartitionStartingLocation +
-    ShortAd->ExtentPosition;
+  return (UINT64)PartitionDesc->PartitionStartingLocation -
+    Volume->MainVdsStartLocation + ShortAd->ExtentPosition;
 }
 
 //
@@ -311,36 +276,39 @@ EFI_STATUS
 FindFileSetDescriptor (
   IN   EFI_BLOCK_IO_PROTOCOL    *BlockIo,
   IN   EFI_DISK_IO_PROTOCOL     *DiskIo,
-  IN   UDF_VOLUME_INFO          *Volume,
-  IN   UINTN                    LogicalVolDescNum,
-  OUT  UDF_FILE_SET_DESCRIPTOR  *FileSetDesc
+  IN   UDF_VOLUME_INFO          *Volume
   )
 {
   EFI_STATUS                     Status;
   UINT64                         Lsn;
   UDF_LOGICAL_VOLUME_DESCRIPTOR  *LogicalVolDesc;
 
-  LogicalVolDesc = Volume->LogicalVolDescs[LogicalVolDescNum];
+  LogicalVolDesc = &Volume->LogicalVolDesc;
   Lsn = GetLongAdLsn (Volume, &LogicalVolDesc->LogicalVolumeContentsUse);
 
   //
-  // Read extent (Long Ad).
+  // As per UDF 2.60 specification:
+  //
+  // There shall be exactly one File Set Descriptor recorded per Logical
+  // Volume.
+  //
+  // Read disk block
   //
   Status = DiskIo->ReadDisk (
     DiskIo,
     BlockIo->Media->MediaId,
     MultU64x32 (Lsn, LogicalVolDesc->LogicalBlockSize),
-    sizeof (UDF_FILE_SET_DESCRIPTOR),
-    (VOID *)FileSetDesc
+    sizeof (Volume->FileSetDesc),
+    &Volume->FileSetDesc
     );
   if (EFI_ERROR (Status)) {
     return Status;
   }
 
   //
-  // Check if the read extent contains a valid FSD's tag identifier.
+  // Check if read block is a File Set Descriptor
   //
-  if (!IS_FSD (FileSetDesc)) {
+  if (!IS_FSD (&Volume->FileSetDesc)) {
     return EFI_VOLUME_CORRUPTED;
   }
 
@@ -348,71 +316,6 @@ FindFileSetDescriptor (
 }
 
 //
-// Get all File Set Descriptors for each Logical Volume Descriptor.
-//
-EFI_STATUS
-GetFileSetDescriptors (
-  IN      EFI_BLOCK_IO_PROTOCOL  *BlockIo,
-  IN      EFI_DISK_IO_PROTOCOL   *DiskIo,
-  IN OUT  UDF_VOLUME_INFO        *Volume
-  )
-{
-  EFI_STATUS               Status;
-  UINTN                    Index;
-  UDF_FILE_SET_DESCRIPTOR  *FileSetDesc;
-  UINTN                    Count;
-
-  Volume->FileSetDescs =
-    (UDF_FILE_SET_DESCRIPTOR **)AllocateZeroPool (
-      Volume->LogicalVolDescsNo * sizeof (UDF_FILE_SET_DESCRIPTOR));
-  if (Volume->FileSetDescs == NULL) {
-    return EFI_OUT_OF_RESOURCES;
-  }
-
-  for (Index = 0; Index < Volume->LogicalVolDescsNo; Index++) {
-    FileSetDesc = AllocateZeroPool (sizeof (UDF_FILE_SET_DESCRIPTOR));
-    if (FileSetDesc == NULL) {
-      Status = EFI_OUT_OF_RESOURCES;
-      goto Error_Alloc_Fsd;
-    }
-
-    //
-    // Find a FSD for this LVD.
-    //
-    Status = FindFileSetDescriptor (
-      BlockIo,
-      DiskIo,
-      Volume,
-      Index,
-      FileSetDesc
-      );
-    if (EFI_ERROR (Status)) {
-      goto Error_Find_Fsd;
-    }
-
-    //
-    // Got one. Save it.
-    //
-    Volume->FileSetDescs[Index] = FileSetDesc;
-  }
-
-  Volume->FileSetDescsNo = Volume->LogicalVolDescsNo;
-  return EFI_SUCCESS;
-
-Error_Find_Fsd:
-  Count = Index + 1;
-  for (Index = 0; Index < Count; Index++) {
-    FreePool ((VOID *)Volume->FileSetDescs[Index]);
-  }
-
-  FreePool ((VOID *)Volume->FileSetDescs);
-  Volume->FileSetDescs = NULL;
-
-Error_Alloc_Fsd:
-  return Status;
-}
-
-//
 // Read Volume and File Structure on an UDF file system.
 //
 EFI_STATUS
@@ -424,28 +327,37 @@ ReadVolumeFileStructure (
 {
   EFI_STATUS                            Status;
   UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER  AnchorPoint;
+  UDF_EXTENT_AD                         *ExtentAd;
 
   //
-  // Find an AVDP.
+  // Find Anchor Volume Descriptor Pointer
   //
   Status = FindAnchorVolumeDescriptorPointer (
     BlockIo,
     DiskIo,
     &AnchorPoint
     );
+  ASSERT_EFI_ERROR (Status);
   if (EFI_ERROR (Status)) {
     return Status;
   }
 
   //
-  // AVDP has been found. Start MVDS.
+  // Save Main VDS start block number
+  //
+  ExtentAd = &AnchorPoint.MainVolumeDescriptorSequenceExtent;
+
+  Volume->MainVdsStartLocation = (UINT64)ExtentAd->ExtentLocation;
+
+  //
+  // Start Main Volume Descriptor Sequence.
   //
   Status = StartMainVolumeDescriptorSequence (
     BlockIo,
     DiskIo,
-    &AnchorPoint,
     Volume
     );
+  ASSERT_EFI_ERROR (Status);
   if (EFI_ERROR (Status)) {
     return Status;
   }
@@ -699,6 +611,7 @@ GetAllocationDescriptorLsn (
     return GetLongAdLsn (Volume, (UDF_LONG_ALLOCATION_DESCRIPTOR *)Ad);
   } else if (RecordingFlags == SHORT_ADS_SEQUENCE) {
     return GetShortAdLsn (
+      Volume,
       GetPdFromLongAd (Volume, ParentIcb),
       (UDF_SHORT_ALLOCATION_DESCRIPTOR *)Ad
       );
@@ -740,7 +653,7 @@ GetAedAdsOffset (
     return EFI_OUT_OF_RESOURCES;
   }
 
-  LogicalBlockSize = LV_BLOCK_SIZE (Volume, UDF_DEFAULT_LV_NUM);
+  LogicalBlockSize = Volume->LogicalVolDesc.LogicalBlockSize;
 
   //
   // Read extent.
@@ -890,7 +803,7 @@ ReadFile (
   UINT32                  ExtentLength;
   UDF_FE_RECORDING_FLAGS  RecordingFlags;
 
-  LogicalBlockSize  = LV_BLOCK_SIZE (Volume, UDF_DEFAULT_LV_NUM);
+  LogicalBlockSize  = Volume->LogicalVolDesc.LogicalBlockSize;
   DoFreeAed         = FALSE;
 
   //
@@ -1355,22 +1268,26 @@ ReadUdfVolumeInformation (
 {
   EFI_STATUS Status;
 
+  //
+  // Read all necessary UDF volume information and keep it private to the driver
+  //
   Status = ReadVolumeFileStructure (
     BlockIo,
     DiskIo,
     Volume
     );
+  ASSERT_EFI_ERROR (Status);
   if (EFI_ERROR (Status)) {
     return Status;
   }
 
-  Status = GetFileSetDescriptors (
-    BlockIo,
-    DiskIo,
-    Volume
-    );
+  //
+  // Find File Set Descriptor
+  //
+  Status = FindFileSetDescriptor (BlockIo, DiskIo, Volume);
+  ASSERT_EFI_ERROR (Status);
   if (EFI_ERROR (Status)) {
-    CleanupVolumeInformation (Volume);
+    return Status;
   }
 
   return Status;
@@ -1407,7 +1324,7 @@ FindRootDirectory (
     BlockIo,
     DiskIo,
     Volume,
-    &Volume->FileSetDescs[0]->RootDirectoryIcb,
+    &Volume->FileSetDesc.RootDirectoryIcb,
     &File->FileEntry
     );
   if (EFI_ERROR (Status)) {
@@ -1424,7 +1341,7 @@ FindRootDirectory (
     L"\\",
     NULL,
     &Parent,
-    &Volume->FileSetDescs[0]->RootDirectoryIcb,
+    &Volume->FileSetDesc.RootDirectoryIcb,
     File
     );
   if (EFI_ERROR (Status)) {
@@ -1465,7 +1382,7 @@ FindFileEntry (
   UINT32      LogicalBlockSize;
 
   Lsn               = GetLongAdLsn (Volume, Icb);
-  LogicalBlockSize  = LV_BLOCK_SIZE (Volume, UDF_DEFAULT_LV_NUM);
+  LogicalBlockSize  = Volume->LogicalVolDesc.LogicalBlockSize;
 
   *FileEntry = AllocateZeroPool (Volume->FileEntrySize);
   if (*FileEntry == NULL) {
@@ -1959,43 +1876,6 @@ Error_Find_File:
 }
 
 /**
-  Clean up in-memory UDF volume information.
-
-  @param[in] Volume Volume information pointer.
-
-**/
-VOID
-CleanupVolumeInformation (
-  IN UDF_VOLUME_INFO *Volume
-  )
-{
-  UINTN Index;
-
-  if (Volume->LogicalVolDescs != NULL) {
-    for (Index = 0; Index < Volume->LogicalVolDescsNo; Index++) {
-      FreePool ((VOID *)Volume->LogicalVolDescs[Index]);
-    }
-    FreePool ((VOID *)Volume->LogicalVolDescs);
-  }
-
-  if (Volume->PartitionDescs != NULL) {
-    for (Index = 0; Index < Volume->PartitionDescsNo; Index++) {
-      FreePool ((VOID *)Volume->PartitionDescs[Index]);
-    }
-    FreePool ((VOID *)Volume->PartitionDescs);
-  }
-
-  if (Volume->FileSetDescs != NULL) {
-    for (Index = 0; Index < Volume->FileSetDescsNo; Index++) {
-      FreePool ((VOID *)Volume->FileSetDescs[Index]);
-    }
-    FreePool ((VOID *)Volume->FileSetDescs);
-  }
-
-  ZeroMem ((VOID *)Volume, sizeof (UDF_VOLUME_INFO));
-}
-
-/**
   Clean up in-memory UDF file information.
 
   @param[in] File File information pointer.
@@ -2249,91 +2129,100 @@ GetVolumeSize (
   OUT  UINT64                 *FreeSpaceSize
   )
 {
-  UDF_EXTENT_AD                 ExtentAd;
-  UINT32                        LogicalBlockSize;
-  UINT64                        Lsn;
-  EFI_STATUS                    Status;
-  UDF_LOGICAL_VOLUME_INTEGRITY  *LogicalVolInt;
-  UINTN                         Index;
-  UINTN                         Length;
-  UINT32                        LsnsNo;
-
-  *VolumeSize     = 0;
-  *FreeSpaceSize  = 0;
-
-  for (Index = 0; Index < Volume->LogicalVolDescsNo; Index++) {
-    CopyMem ((VOID *)&ExtentAd,
-             (VOID *)&Volume->LogicalVolDescs[Index]->IntegritySequenceExtent,
-             sizeof (UDF_EXTENT_AD));
-    if (ExtentAd.ExtentLength == 0) {
-      continue;
-    }
+  EFI_STATUS                     Status;
+  UDF_LOGICAL_VOLUME_DESCRIPTOR  *LogicalVolDesc;
+  UDF_EXTENT_AD                  *ExtentAd;
+  UINT64                         Lsn;
+  UINT32                         LogicalBlockSize;
+  UDF_LOGICAL_VOLUME_INTEGRITY   *LogicalVolInt;
+  UINTN                          Index;
+  UINTN                          Length;
+  UINT32                         LsnsNo;
 
-    LogicalBlockSize = LV_BLOCK_SIZE (Volume, Index);
+  LogicalVolDesc = &Volume->LogicalVolDesc;
 
-  Read_Next_Sequence:
-    LogicalVolInt = (UDF_LOGICAL_VOLUME_INTEGRITY *)
-      AllocatePool (ExtentAd.ExtentLength);
-    if (LogicalVolInt == NULL) {
-      return EFI_OUT_OF_RESOURCES;
-    }
+  ExtentAd = &LogicalVolDesc->IntegritySequenceExtent;
 
-    Lsn = (UINT64)ExtentAd.ExtentLocation;
+  if (ExtentAd->ExtentLength == 0) {
+    return EFI_VOLUME_CORRUPTED;
+  }
 
-    Status = DiskIo->ReadDisk (
-      DiskIo,
-      BlockIo->Media->MediaId,
-      MultU64x32 (Lsn, LogicalBlockSize),
-      ExtentAd.ExtentLength,
-      (VOID *)LogicalVolInt
-      );
-    if (EFI_ERROR (Status)) {
-      FreePool ((VOID *)LogicalVolInt);
-      return Status;
-    }
+  LogicalVolInt = AllocatePool (ExtentAd->ExtentLength);
+  if (LogicalVolInt == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
 
-    if (!IS_LVID (LogicalVolInt)) {
-      FreePool ((VOID *)LogicalVolInt);
-      return EFI_VOLUME_CORRUPTED;
-    }
+  //
+  // Get location of Logical Volume Integrity Descriptor
+  //
+  Lsn = (UINT64)ExtentAd->ExtentLocation - Volume->MainVdsStartLocation;
 
-    Length = LogicalVolInt->NumberOfPartitions;
-    for (Index = 0; Index < Length; Index += sizeof (UINT32)) {
-      LsnsNo = *(UINT32 *)((UINT8 *)LogicalVolInt->Data + Index);
-      if (LsnsNo == 0xFFFFFFFFUL) {
-        //
-        // Size not specified.
-        //
-        continue;
-      }
+  LogicalBlockSize = LogicalVolDesc->LogicalBlockSize;
 
-      *FreeSpaceSize += MultU64x32 ((UINT64)LsnsNo, LogicalBlockSize);
-    }
+  //
+  // Read disk block
+  //
+  Status = DiskIo->ReadDisk (
+    DiskIo,
+    BlockIo->Media->MediaId,
+    MultU64x32 (Lsn, LogicalBlockSize),
+    ExtentAd->ExtentLength,
+    LogicalVolInt
+    );
+  if (EFI_ERROR (Status)) {
+    goto Out_Free;
+  }
 
-    Length = (LogicalVolInt->NumberOfPartitions * sizeof (UINT32)) << 1;
-    for (; Index < Length; Index += sizeof (UINT32)) {
-      LsnsNo = *(UINT32 *)((UINT8 *)LogicalVolInt->Data + Index);
-      if (LsnsNo == 0xFFFFFFFFUL) {
-        //
-        // Size not specified.
-        //
-        continue;
-      }
+  //
+  // Check if read block is a Logical Volume Integrity Descriptor
+  //
+  if (!IS_LVID (LogicalVolInt)) {
+    Status = EFI_VOLUME_CORRUPTED;
+    goto Out_Free;
+  }
 
-      *VolumeSize += MultU64x32 ((UINT64)LsnsNo, LogicalBlockSize);
-    }
+  *VolumeSize = 0;
+  *FreeSpaceSize = 0;
 
-    CopyMem ((VOID *)&ExtentAd,(VOID *)&LogicalVolInt->NextIntegrityExtent,
-             sizeof (UDF_EXTENT_AD));
-    if (ExtentAd.ExtentLength > 0) {
-      FreePool ((VOID *)LogicalVolInt);
-      goto Read_Next_Sequence;
+  Length = LogicalVolInt->NumberOfPartitions;
+  for (Index = 0; Index < Length; Index += sizeof (UINT32)) {
+    LsnsNo = *(UINT32 *)((UINT8 *)LogicalVolInt->Data + Index);
+    //
+    // Check if size is not specified
+    //
+    if (LsnsNo == 0xFFFFFFFFUL) {
+      continue;
     }
+    //
+    // Accumulate free space size
+    //
+    *FreeSpaceSize += MultU64x32 ((UINT64)LsnsNo, LogicalBlockSize);
+  }
 
-    FreePool ((VOID *)LogicalVolInt);
+  Length = LogicalVolInt->NumberOfPartitions * sizeof (UINT32) * 2;
+  for (; Index < Length; Index += sizeof (UINT32)) {
+    LsnsNo = *(UINT32 *)((UINT8 *)LogicalVolInt->Data + Index);
+    //
+    // Check if size is not specified
+    //
+    if (LsnsNo == 0xFFFFFFFFUL) {
+      continue;
+    }
+    //
+    // Accumulate used volume space
+    //
+    *VolumeSize += MultU64x32 ((UINT64)LsnsNo, LogicalBlockSize);
   }
 
-  return EFI_SUCCESS;
+  Status = EFI_SUCCESS;
+
+Out_Free:
+  //
+  // Free Logical Volume Integrity Descriptor
+  //
+  FreePool (LogicalVolInt);
+
+  return Status;
 }
 
 /**
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/Udf.c b/MdeModulePkg/Universal/Disk/UdfDxe/Udf.c
index 49dc7077b7..d4163b89ca 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/Udf.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/Udf.c
@@ -276,13 +276,6 @@ UdfDriverBindingStop (
       NULL
       );
 
-    //
-    // Check if there's any open file. If so, clean them up.
-    //
-    if (PrivFsData->OpenFiles > 0) {
-      CleanupVolumeInformation (&PrivFsData->Volume);
-    }
-
     FreePool ((VOID *)PrivFsData);
   }
 
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/Udf.h b/MdeModulePkg/Universal/Disk/UdfDxe/Udf.h
index 240d420ff5..c5f83914d8 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/Udf.h
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/Udf.h
@@ -49,16 +49,8 @@
     { 0x89, 0x56, 0x73, 0xCD, 0xA3, 0x26, 0xCD, 0x0A }  \
   }
 
-#define UDF_DEFAULT_LV_NUM 0
-
 #define IS_PVD(_Pointer) \
   ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 1))
-#define IS_PD(_Pointer) \
-  ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 5))
-#define IS_LVD(_Pointer) \
-  ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 6))
-#define IS_TD(_Pointer) \
-  ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 8))
 #define IS_FSD(_Pointer) \
   ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 256))
 #define IS_FE(_Pointer) \
@@ -152,14 +144,8 @@ typedef enum {
 #define IS_VALID_COMPRESSION_ID(_CompId) \
   ((BOOLEAN)((_CompId) == 8 || (_CompId) == 16))
 
-#define LV_BLOCK_SIZE(_Vol, _LvNum) \
-  (_Vol)->LogicalVolDescs[(_LvNum)]->LogicalBlockSize
-
 #define UDF_STANDARD_IDENTIFIER_LENGTH   5
 
-#define LV_UDF_REVISION(_Lv) \
-  *(UINT16 *)(UINTN)(_Lv)->DomainIdentifier.IdentifierSuffix
-
 #pragma pack(1)
 
 typedef struct {
@@ -186,17 +172,6 @@ typedef struct {
 #pragma pack(1)
 
 typedef struct {
-  UINT8           CharacterSetType;
-  UINT8           CharacterSetInfo[63];
-} UDF_CHAR_SPEC;
-
-typedef struct {
-  UINT8           Flags;
-  UINT8           Identifier[23];
-  UINT8           IdentifierSuffix[8];
-} UDF_ENTITY_ID;
-
-typedef struct {
   UINT16          TypeAndTimezone;
   INT16           Year;
   UINT8           Month;
@@ -210,17 +185,6 @@ typedef struct {
 } UDF_TIMESTAMP;
 
 typedef struct {
-  UINT32        LogicalBlockNumber;
-  UINT16        PartitionReferenceNumber;
-} UDF_LB_ADDR;
-
-typedef struct {
-  UINT32                           ExtentLength;
-  UDF_LB_ADDR                      ExtentLocation;
-  UINT8                            ImplementationUse[6];
-} UDF_LONG_ALLOCATION_DESCRIPTOR;
-
-typedef struct {
   UDF_DESCRIPTOR_TAG                 DescriptorTag;
   UINT32                             PrevAllocationExtentDescriptor;
   UINT32                             LengthOfAllocationDescriptors;
@@ -235,37 +199,6 @@ typedef struct {
 } UDF_VOLUME_DESCRIPTOR;
 
 typedef struct {
-  UDF_DESCRIPTOR_TAG         DescriptorTag;
-  UINT32                     VolumeDescriptorSequenceNumber;
-  UINT16                     PartitionFlags;
-  UINT16                     PartitionNumber;
-  UDF_ENTITY_ID              PartitionContents;
-  UINT8                      PartitionContentsUse[128];
-  UINT32                     AccessType;
-  UINT32                     PartitionStartingLocation;
-  UINT32                     PartitionLength;
-  UDF_ENTITY_ID              ImplementationIdentifier;
-  UINT8                      ImplementationUse[128];
-  UINT8                      Reserved[156];
-} UDF_PARTITION_DESCRIPTOR;
-
-typedef struct {
-  UDF_DESCRIPTOR_TAG              DescriptorTag;
-  UINT32                          VolumeDescriptorSequenceNumber;
-  UDF_CHAR_SPEC                   DescriptorCharacterSet;
-  UINT8                           LogicalVolumeIdentifier[128];
-  UINT32                          LogicalBlockSize;
-  UDF_ENTITY_ID                   DomainIdentifier;
-  UDF_LONG_ALLOCATION_DESCRIPTOR  LogicalVolumeContentsUse;
-  UINT32                          MapTableLength;
-  UINT32                          NumberOfPartitionMaps;
-  UDF_ENTITY_ID                   ImplementationIdentifier;
-  UINT8                           ImplementationUse[128];
-  UDF_EXTENT_AD                   IntegritySequenceExtent;
-  UINT8                           PartitionMaps[6];
-} UDF_LOGICAL_VOLUME_DESCRIPTOR;
-
-typedef struct {
   UDF_DESCRIPTOR_TAG             DescriptorTag;
   UDF_TIMESTAMP                  RecordingDateTime;
   UINT32                         IntegrityType;
@@ -389,12 +322,10 @@ typedef struct {
 // UDF filesystem driver's private data
 //
 typedef struct {
-  UDF_LOGICAL_VOLUME_DESCRIPTOR  **LogicalVolDescs;
-  UINTN                          LogicalVolDescsNo;
-  UDF_PARTITION_DESCRIPTOR       **PartitionDescs;
-  UINTN                          PartitionDescsNo;
-  UDF_FILE_SET_DESCRIPTOR        **FileSetDescs;
-  UINTN                          FileSetDescsNo;
+  UINT64                         MainVdsStartLocation;
+  UDF_LOGICAL_VOLUME_DESCRIPTOR  LogicalVolDesc;
+  UDF_PARTITION_DESCRIPTOR       PartitionDesc;
+  UDF_FILE_SET_DESCRIPTOR        FileSetDesc;
   UINTN                          FileEntrySize;
 } UDF_VOLUME_INFO;
 
@@ -883,17 +814,6 @@ ResolveSymlink (
   );
 
 /**
-  Clean up in-memory UDF volume information.
-
-  @param[in] Volume Volume information pointer.
-
-**/
-VOID
-CleanupVolumeInformation (
-  IN UDF_VOLUME_INFO *Volume
-  );
-
-/**
   Clean up in-memory UDF file information.
 
   @param[in] File File information pointer.
-- 
2.11.0

Re: [PATCH 0/3] UDF partition driver fix

[Laszlo Ersek]

Hi Paulo,

On 09/16/17 23:36, Paulo Alcantara wrote:
> This series fixes an UDF issue with Partition driver as discussed in ML:
> 
> https://lists.01.org/pipermail/edk2-devel/2017-September/014694.html
> 
> Thanks!
> Paulo
> 
> Repo:   https://github.com/pcacjr/edk2.git
> Branch: udf-partition-fix
> 
> Paulo Alcantara (3):
>   MdePkg: Add UDF volume structure definitions
>   MdeModulePkg/PartitionDxe: Fix creation of UDF logical partition
>   MdeModulePkg/UdfDxe: Rework driver to support PartitionDxe changes
> 
>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c     | 307 +++++++++++-
>  MdeModulePkg/Universal/Disk/UdfDxe/File.c          |  13 +-
>  .../Universal/Disk/UdfDxe/FileSystemOperations.c   | 525 ++++++++-------------
>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.c           |   7 -
>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.h           |  88 +---
>  MdePkg/Include/IndustryStandard/Udf.h              |  63 +++
>  6 files changed, 560 insertions(+), 443 deletions(-)
> 

Thank you very much for submitting this patchset quickly. I hope it will
work out, and we won't need the PartitionExperimentalDxe.inf file!

I have some trivial process-level suggestions:

* when submitting a patchset, please collect the Cc: tags from all the
commit messages, and add them to the cover letter manually. This way
everybody you CC on at least some of the patches will get the cover
letter too, presonally.

This matters because otherwise replies to the blurb will also miss those
people, personally. (I'm now adding everyone manually.)

* Because edk2 uses long directory and file names, the diffstats are
frequently truncated like above (see "..."). You can avoid this if you
format the patches like this:

  --stat=1000 --stat-graph-width=20

this will make the pathname column just as wide as necessary, and will
also keep the chart to the right reasonably narrow.

* It's probably best to include a reference to
<https://bugzilla.tianocore.org/show_bug.cgi?id=707> in the commit
messages (in particular patch #2).

* Once you post a patchset for a TianoCore BZ, it's useful to link the
series (from the mailing list archive) in the BZ itself.


Regarding the code itself, I don't think I can help here in any sensible
way. (If UDF support were located under OvmfPkg, I would totally
consider you the owner of those files, verify your patches for them on a
formal level only, and if that part was fine, I'd give an Acked-by.)

Thanks!
Laszlo

Re: [PATCH 0/3] UDF partition driver fix

[Yao, Jiewen]

Thank you Paulo, to provide a fix for this driver.

I do not have comment for this specific patch. I would defer the review work to Star and Ruiyu.
I do have some general question for the new UDF support and I would like to know more detail about the quality level.

As we are seeing some issues in the new UDF driver, would you please share what test you have done for the UDF support? (Not only for this patch, but also for the UDF partition and UDF file system which are already checked in)

I ask this specially, because UDF support (partition and file system) adds a brand new group of external input for the UEFI BIOS. For a long time, we are monitoring all the external input.
Per our security model, the external input means the input by an end user. The known external includes but not limited to UEFI image (OROM/OS Loader), Capsule Image, Recovery Image, file system, partition, network packet, variable, etc.

UDF is a new one, because with the new UDF support, now a malicious user may insert a mal-format UDF CDROM to the system and try to break the system. As such, we need evaluate it.


To be specific, would you please share:

1)       Which UDF spec these 2 drivers (FS and partition) are following?
I have seen you mentioned the header file follows "(revisions 1.02 through 2.60)". But I am not sure about the driver.

I found you mentioned:

Originally the driver was written to support UDF file systems as

specified by OSTA Universal Disk Format Specification 2.60. However,

some Windows 10 Enterprise ISO (UDF bridge) images that I tested

supported a revision of 1.02 thus I had to rework the driver a little

bit to support such revision as well.

Do you mean you only support 1.02 and 2.6 in driver, or you support 1.02 through 2.6?


2)       Which UDF function is supported? And more important, which UDF function is NOT supported?
I have seen "Compliance" section in UDF spec, and it lists some optional feature, such as multi-volume, multi-partition, multisession, file name translation, backward read, backward write, etc.

I also have interest to know the support level of current existing CDROM, and existing UDF driver in OS (such as Windows, or Linux). How many optional feature are implemented?

I ask this, because we want to understand how we declare the support level of this UEFI UDF driver. If we just say we support UDF, then naive people may believe we support everything. :)


3)       Which compatibility test has been done?
I am sorry that I cannot find the first version patch. I fund you mentioned Win10 ISO is tried in V2. Any more?
We would like to know how many existing OS installation CDROM (or any other CDROM) has been tried. Such as Linux (RedHat, Ubuntu, Suse, etc), or Windows (Win7, Win8, Win10)?
The more detail, the better. May a list.


4)       The last but not least important, which negative test (security test) has been done?
Have you run some fuzzing test?
Have you tried a mal-format UDF disc? For example, with bad offset or length?
Have you test the integer overflow / buffer over flow handing in the code?

NOTE: we should not use ASSERT to handle such error.
When I review the code, I found below:

    Status = ReadFileData (
      BlockIo,
      DiskIo,
      Volume,
      Parent,
      PrivFileData->FileSize,
      &PrivFileData->FilePosition,
      Buffer,
      &BufferSizeUint64
      );
    ASSERT (BufferSizeUint64 <= MAX_UINTN);
    *BufferSize = (UINTN)BufferSizeUint64;

I am not sure if we can use ASSERT (BufferSizeUint64 <= MAX_UINTN);
Can a malicious user construct a bad UDF and make BufferSizeUint64 > MAX_UINTN?
Does it might happen? Or never happen?

We documented Appendix B - EDKII code review top 5 in https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Security_Design_Guide_in_EDK_II.pdf
3 of them are matched in these partition and file system drivers. I quote below:
===============================
If the code consumes input from an untrusted source or region,
Ensure that the input is rigorously and adequately validated.
Verify buffer overflow is handled. Avoid integer overflow.
Try to use subtraction instead of addition and division instead of multiplication.
Verify that ASSERT is used properly.
ASSERT is used to catch conditions that should never happen. If something might happen, use error handling instead. We can use both ASSERT and error handling to facilitate debugging - ASSERT allows for earlier detection and isolation of several classes of issues. [McConnell]
===============================

It is just a reminder. If you are already following that, it will be great. Please let us now.


I take a glance of UDF 2.6 specification, but I do not have chance to read all content at this moment.
If I asked some stupid question , please feel free to correct me.


All in all, we hope to understand the current quality level of the UDF partition support and UDF file system.
If you can help to provide the detail, it may help us to evaluate.



Thank you
Yao Jiewen


From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Sunday, September 17, 2017 6:17 AM
To: Paulo Alcantara <pcacjr@zytor.com>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] [PATCH 0/3] UDF partition driver fix

Hi Paulo,

On 09/16/17 23:36, Paulo Alcantara wrote:
> This series fixes an UDF issue with Partition driver as discussed in ML:
>
> https://lists.01.org/pipermail/edk2-devel/2017-September/014694.html
>
> Thanks!
> Paulo
>
> Repo:   https://github.com/pcacjr/edk2.git
> Branch: udf-partition-fix
>
> Paulo Alcantara (3):
>   MdePkg: Add UDF volume structure definitions
>   MdeModulePkg/PartitionDxe: Fix creation of UDF logical partition
>   MdeModulePkg/UdfDxe: Rework driver to support PartitionDxe changes
>
>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c     | 307 +++++++++++-
>  MdeModulePkg/Universal/Disk/UdfDxe/File.c          |  13 +-
>  .../Universal/Disk/UdfDxe/FileSystemOperations.c   | 525 ++++++++-------------
>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.c           |   7 -
>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.h           |  88 +---
>  MdePkg/Include/IndustryStandard/Udf.h              |  63 +++
>  6 files changed, 560 insertions(+), 443 deletions(-)
>

Thank you very much for submitting this patchset quickly. I hope it will
work out, and we won't need the PartitionExperimentalDxe.inf file!

I have some trivial process-level suggestions:

* when submitting a patchset, please collect the Cc: tags from all the
commit messages, and add them to the cover letter manually. This way
everybody you CC on at least some of the patches will get the cover
letter too, presonally.

This matters because otherwise replies to the blurb will also miss those
people, personally. (I'm now adding everyone manually.)

* Because edk2 uses long directory and file names, the diffstats are
frequently truncated like above (see "..."). You can avoid this if you
format the patches like this:

  --stat=1000 --stat-graph-width=20

this will make the pathname column just as wide as necessary, and will
also keep the chart to the right reasonably narrow.

* It's probably best to include a reference to
<https://bugzilla.tianocore.org/show_bug.cgi?id=707> in the commit
messages (in particular patch #2).

* Once you post a patchset for a TianoCore BZ, it's useful to link the
series (from the mailing list archive) in the BZ itself.


Regarding the code itself, I don't think I can help here in any sensible
way. (If UDF support were located under OvmfPkg, I would totally
consider you the owner of those files, verify your patches for them on a
formal level only, and if that part was fine, I'd give an Acked-by.)

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/3] UDF partition driver fix

[Laszlo Ersek]

Hi Jiewen,

I agree these are important questions; even earlier I noticed the
following remark in "PartitionDxe.inf":

#  Caution: This module requires additional review when modified.
#  This driver will have external input - disk partition.
#  This external input must be validated carefully to avoid security issue like
#  buffer overflow, integer overflow.

I'll let Paulo answer your questions.

I'll just comment on one thing because your specific question refers to
code that I recommended.

On 09/17/17 01:52, Yao, Jiewen wrote:

> 4)       The last but not least important, which negative test
> (security test) has been done?
> Have you run some fuzzing test?
> Have you tried a mal-format UDF disc? For example, with bad offset or
> length?
> Have you test the integer overflow / buffer over flow handing in the
> code?
>
> NOTE: we should not use ASSERT to handle such error.
> When I review the code, I found below:
>
>     Status = ReadFileData (
>       BlockIo,
>       DiskIo,
>       Volume,
>       Parent,
>       PrivFileData->FileSize,
>       &PrivFileData->FilePosition,
>       Buffer,
>       &BufferSizeUint64
>       );
>     ASSERT (BufferSizeUint64 <= MAX_UINTN);
>     *BufferSize = (UINTN)BufferSizeUint64;
>
> I am not sure if we can use ASSERT (BufferSizeUint64 <= MAX_UINTN);
> Can a malicious user construct a bad UDF and make BufferSizeUint64 >
> MAX_UINTN?
> Does it might happen? Or never happen?
>
> We documented Appendix B - EDKII code review top 5 in
> https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Security_Design_Guide_in_EDK_II.pdf
> 3 of them are matched in these partition and file system drivers. I
> quote below:
> ===============================
> If the code consumes input from an untrusted source or region,
> Ensure that the input is rigorously and adequately validated.
> Verify buffer overflow is handled. Avoid integer overflow.
> Try to use subtraction instead of addition and division instead of
> multiplication.
> Verify that ASSERT is used properly.
> ASSERT is used to catch conditions that should never happen. If
> something might happen, use error handling instead. We can use both
> ASSERT and error handling to facilitate debugging - ASSERT allows for
> earlier detection and isolation of several classes of issues.
> [McConnell]
> ===============================

You can find the discussion about the code above here:

http://mid.mail-archive.com/8ca69ac9-4f5a-3aa9-f150-844b0aeeb898@redhat.com

I can describe it in more detail here:

The UdfRead() function, which implements EFI_FILE_PROTOCOL.Read(), takes
an IN OUT parameter called BufferSize:

typedef
EFI_STATUS
(EFIAPI *EFI_FILE_READ)(
  IN EFI_FILE_PROTOCOL        *This,
  IN OUT UINTN                *BufferSize,
  OUT VOID                    *Buffer
  );

BufferSize points to an UINTN variable. On input BufferSize says how
much data the caller is trying to read, and on output it tells the
caller how much data was actually read.

In the UdfRead() function, we pass the pointer to a helper function
called ReadFileData(). ReadFileData() takes a similar IN OUT BufferSize
parameter, but in ReadFileData() the pointer is to UINT64, not UINTN:

EFI_STATUS
ReadFileData (
  IN      EFI_BLOCK_IO_PROTOCOL  *BlockIo,
  IN      EFI_DISK_IO_PROTOCOL   *DiskIo,
  IN      UDF_VOLUME_INFO        *Volume,
  IN      UDF_FILE_INFO          *File,
  IN      UINT64                 FileSize,
  IN OUT  UINT64                 *FilePosition,
  IN OUT  VOID                   *Buffer,
  IN OUT  UINT64                 *BufferSize
  )

Therefore we cannot pass the original pointer directly, because in
32-bit builds, ReadFileData() would access 64 bits through the pointer,
even though the caller of UdfRead() originally allocated only 32 bits
for the outermost BufferSize variable.

Therefore, in UdfRead(), we have a local variable

  UINT64                          BufferSizeUint64;

and we use it like this:

    BufferSizeUint64 = *BufferSize;

    Status = ReadFileData (
      BlockIo,
      DiskIo,
      Volume,
      Parent,
      PrivFileData->FileSize,
      &PrivFileData->FilePosition,
      Buffer,
      &BufferSizeUint64
      );
    ASSERT (BufferSizeUint64 <= MAX_UINTN);
    *BufferSize = (UINTN)BufferSizeUint64;

First we set the helper variable to *BufferSize, from the caller. In
64-bit builds, UINTN is UINT64, hence this is a UINT64-to-UINT64
assignment. In 32-bit builds, UINTN is UINT32, hence this is a
UINT32-to-UINT64 assignment.

Then we call ReadFileData() with a pointer to the helper variable. This
is safe because the helper variable has enough room (64 bits) so that
ReadFileData() will not access data beyond it.

Finally, we must put back the result from BufferSizeUint64, to the
caller's (*BufferSize) object. In 64-bit builds, this is a
UINT64-to-UINT64 assignment, so that is safe. However, in 32-bit builds,
this is a UINT64-to-UINT32 assignment, and we must prevent the value
from being truncated.

The insight here is that ReadFileData() must never *increase* the value
-- it may read exactly as much as, or less than, the amount of data
requested, but it must never read more than requested. Therefore, given
that the input value of BufferSizeUint64 was *BufferSize, i.e., a UINTN,
ReadFileData() guarantees that the output value, which may never be
larger than the input value, will also fit into a UINTN. This is why the
ASSERT() is appropriate -- if this invariant were broken, then it would
not be a consequence of user input (that is, not caused by a
user-provided UDF filesystem), but a bug in ReadFileData().

The ASSERT() states that the conversion to (UINTN) that comes right
after is safe, because it should never occur that ReadFileData() read
more data than requested. The ASSERT() doesn't concern user input --
i.e., filesystem data, or EFI_FILE_PROTOCOL.Read() parameters --, it
concerns the interface contract of the internal ReadFileData() function.
If the ASSERT() ever fires, then there's a bug in ReadFileData().

Thanks
Laszlo

Re: [PATCH 0/3] UDF partition driver fix

[Yao, Jiewen]

Thank you Laszlo. You analysis is great!

Thank you
Yao Jiewen

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Sunday, September 17, 2017 6:07 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; Paulo Alcantara <pcacjr@zytor.com>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] [PATCH 0/3] UDF partition driver fix

Hi Jiewen,

I agree these are important questions; even earlier I noticed the
following remark in "PartitionDxe.inf":

#  Caution: This module requires additional review when modified.
#  This driver will have external input - disk partition.
#  This external input must be validated carefully to avoid security issue like
#  buffer overflow, integer overflow.

I'll let Paulo answer your questions.

I'll just comment on one thing because your specific question refers to
code that I recommended.

On 09/17/17 01:52, Yao, Jiewen wrote:

> 4)       The last but not least important, which negative test
> (security test) has been done?
> Have you run some fuzzing test?
> Have you tried a mal-format UDF disc? For example, with bad offset or
> length?
> Have you test the integer overflow / buffer over flow handing in the
> code?
>
> NOTE: we should not use ASSERT to handle such error.
> When I review the code, I found below:
>
>     Status = ReadFileData (
>       BlockIo,
>       DiskIo,
>       Volume,
>       Parent,
>       PrivFileData->FileSize,
>       &PrivFileData->FilePosition,
>       Buffer,
>       &BufferSizeUint64
>       );
>     ASSERT (BufferSizeUint64 <= MAX_UINTN);
>     *BufferSize = (UINTN)BufferSizeUint64;
>
> I am not sure if we can use ASSERT (BufferSizeUint64 <= MAX_UINTN);
> Can a malicious user construct a bad UDF and make BufferSizeUint64 >
> MAX_UINTN?
> Does it might happen? Or never happen?
>
> We documented Appendix B - EDKII code review top 5 in
> https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Security_Design_Guide_in_EDK_II.pdf
> 3 of them are matched in these partition and file system drivers. I
> quote below:
> ===============================
> If the code consumes input from an untrusted source or region,
> Ensure that the input is rigorously and adequately validated.
> Verify buffer overflow is handled. Avoid integer overflow.
> Try to use subtraction instead of addition and division instead of
> multiplication.
> Verify that ASSERT is used properly.
> ASSERT is used to catch conditions that should never happen. If
> something might happen, use error handling instead. We can use both
> ASSERT and error handling to facilitate debugging - ASSERT allows for
> earlier detection and isolation of several classes of issues.
> [McConnell]
> ===============================

You can find the discussion about the code above here:

http://mid.mail-archive.com/8ca69ac9-4f5a-3aa9-f150-844b0aeeb898@redhat.com

I can describe it in more detail here:

The UdfRead() function, which implements EFI_FILE_PROTOCOL.Read(), takes
an IN OUT parameter called BufferSize:

typedef
EFI_STATUS
(EFIAPI *EFI_FILE_READ)(
  IN EFI_FILE_PROTOCOL        *This,
  IN OUT UINTN                *BufferSize,
  OUT VOID                    *Buffer
  );

BufferSize points to an UINTN variable. On input BufferSize says how
much data the caller is trying to read, and on output it tells the
caller how much data was actually read.

In the UdfRead() function, we pass the pointer to a helper function
called ReadFileData(). ReadFileData() takes a similar IN OUT BufferSize
parameter, but in ReadFileData() the pointer is to UINT64, not UINTN:

EFI_STATUS
ReadFileData (
  IN      EFI_BLOCK_IO_PROTOCOL  *BlockIo,
  IN      EFI_DISK_IO_PROTOCOL   *DiskIo,
  IN      UDF_VOLUME_INFO        *Volume,
  IN      UDF_FILE_INFO          *File,
  IN      UINT64                 FileSize,
  IN OUT  UINT64                 *FilePosition,
  IN OUT  VOID                   *Buffer,
  IN OUT  UINT64                 *BufferSize
  )

Therefore we cannot pass the original pointer directly, because in
32-bit builds, ReadFileData() would access 64 bits through the pointer,
even though the caller of UdfRead() originally allocated only 32 bits
for the outermost BufferSize variable.

Therefore, in UdfRead(), we have a local variable

  UINT64                          BufferSizeUint64;

and we use it like this:

    BufferSizeUint64 = *BufferSize;

    Status = ReadFileData (
      BlockIo,
      DiskIo,
      Volume,
      Parent,
      PrivFileData->FileSize,
      &PrivFileData->FilePosition,
      Buffer,
      &BufferSizeUint64
      );
    ASSERT (BufferSizeUint64 <= MAX_UINTN);
    *BufferSize = (UINTN)BufferSizeUint64;

First we set the helper variable to *BufferSize, from the caller. In
64-bit builds, UINTN is UINT64, hence this is a UINT64-to-UINT64
assignment. In 32-bit builds, UINTN is UINT32, hence this is a
UINT32-to-UINT64 assignment.

Then we call ReadFileData() with a pointer to the helper variable. This
is safe because the helper variable has enough room (64 bits) so that
ReadFileData() will not access data beyond it.

Finally, we must put back the result from BufferSizeUint64, to the
caller's (*BufferSize) object. In 64-bit builds, this is a
UINT64-to-UINT64 assignment, so that is safe. However, in 32-bit builds,
this is a UINT64-to-UINT32 assignment, and we must prevent the value
from being truncated.

The insight here is that ReadFileData() must never *increase* the value
-- it may read exactly as much as, or less than, the amount of data
requested, but it must never read more than requested. Therefore, given
that the input value of BufferSizeUint64 was *BufferSize, i.e., a UINTN,
ReadFileData() guarantees that the output value, which may never be
larger than the input value, will also fit into a UINTN. This is why the
ASSERT() is appropriate -- if this invariant were broken, then it would
not be a consequence of user input (that is, not caused by a
user-provided UDF filesystem), but a bug in ReadFileData().

The ASSERT() states that the conversion to (UINTN) that comes right
after is safe, because it should never occur that ReadFileData() read
more data than requested. The ASSERT() doesn't concern user input --
i.e., filesystem data, or EFI_FILE_PROTOCOL.Read() parameters --, it
concerns the interface contract of the internal ReadFileData() function.
If the ASSERT() ever fires, then there's a bug in ReadFileData().

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/3] UDF partition driver fix

[Paulo Alcantara]

Hi Jiewen,

On 16/09/2017 20:52, Yao, Jiewen wrote:
> Thank you Paulo, to provide a fix for this driver.

No problem! I'm trying to do the best I can in my very short time.

> 
> I do not have comment for this specific patch. I would defer the review 
> work to Star and Ruiyu.

OK.

> 
> I do have some general question for the new UDF support and I would like 
> to know more detail about the quality level.
> 
> As we are seeing some issues in the new UDF driver, would you please 
> share what test you have done for the UDF support? (Not only for this 
> patch, but also for the UDF partition and UDF file system which are 
> already checked in)

I tested it with a Windows 10 Enterprise ISO image (UDF bridge disk 
image). With it, I could test if ElTorito boot still worked, as well as 
if I could list directory and files, print out file contents, etc. in 
its UDF file system.

I also used a 32GiB USB stick by formatting it with 'mkudffs -b 512 
--media-type=hd /dev/sdX' and copied some files to it and performed some 
basic file operations like listening and reading files/symlinks. I built 
a Linux kernel with EFI stub support inside the UDF file system -- it 
booted fine, however the kernel wasn't able to mount the rootfs being an 
UDF file system.

I tried some Fedora images I have but they aren't UDF bridge disk images 
(e.g. only ISO9660 + ElTorito), so couldn't test with them.

There's also a great tool I used to validate the ISO images and see if 
they are complaint with UDF specification: "Philips UDF Conformance 
Tool". You may find it at 
https://www.lscdweb.com/registered/udf_verifier.html

> 
> I ask this specially, because UDF support (partition and file system) 
> adds a brand new group of external input for the UEFI BIOS. For a long 
> time, we are monitoring all the external input.

Yeah, you're right.

> 
> Per our security model, the external input means the input by an end 
> user. The known external includes but not limited to UEFI image (OROM/OS 
> Loader), Capsule Image, Recovery Image, file system, partition, network 
> packet, variable, etc.

Good to know!

> 
> UDF is a new one, because with the new UDF support, now a malicious user 
> may insert a mal-format UDF CDROM to the system and try to break the 
> system. As such, we need evaluate it.

Sure!

> 
> To be specific, would you please share:
> 
> 1)Which UDF spec these 2 drivers (FS and partition) are following?

I'm following the UDF 2.60 specification.

> 
> I have seen you mentioned the header file follows “(revisions 1.02 
> through 2.60)”. But I am not sure about the driver.
> 
> I found you mentioned:
> 
> Originally the driver was written to support UDF file systems as
> 
> specified by OSTA Universal Disk Format Specification 2.60. However,
> 
> some Windows 10 Enterprise ISO (UDF bridge) images that I tested
> 
> supported a revision of 1.02 thus I had to rework the driver a little
> 
> bit to support such revision as well.
> 
> Do you mean you only support 1.02 and 2.6 in driver, or you support 1.02 
> through 2.6?
> 
> 2)Which UDF function is supported? And more important, which UDF 
> function is NOT supported?

The partition + file system drivers should support only 2.60. However, 
as you can see in this patchset, there is a GetPartitionNumber() in 
PartitionDxe, which checks for the UDF revision (1.02 through 2.60) in 
order to retrieve the correct partition number and Partition Descriptor.

For instance, the Windows 10 Enterprise ISO image, by running it with 
the "Philips UDF Conformance Tool", it says "Final UDF Revision range: 
1.02 only", but partition + driver works fine with it because of that 
revision check in GetPartitionNumber() in PartitionDxe.

Anyways, it's UDF 2.60 revision, officially.

> 
> I have seen “Compliance” section in UDF spec, and it lists some optional 
> feature, such as multi-volume, multi-partition, multisession, file name 
> translation, backward read, backward write, etc. >
> 
> I also have interest to know the support level of current existing 
> CDROM, and existing UDF driver in OS (such as Windows, or Linux). How 
> many optional feature are implemented?

None optional feature has been implemented. Only the mandatory ones.

I will list some known limitations that I could remember:

- Partition types other than 1, will not be supported.

- Extended Allocation Descriptors are not supported. However, the spec 
  mentions: "Only Short and Long Allocation Descriptors shall be 
recorded". IIRC, the 'mkudffs' tool in Linux, it may create Extended 
Allocation Descriptors in directories with thousands of files, and the 
UDF conformance tool reports that the UDF file system created by the 
tool is complaint with UDF revision 2.60.

- Only one Logical Volume and Partition Descriptor is supported. See "2. 
Basic Restrictions & Requirements" in UDF 2.60 specification.
- No write support.

There may be other unsupported features I forgot to mention. Sorry. I 
also need to read the unfriendly ECMA-167 and UDF specs again and check 
the remaining unsupported features.

>
> I ask this, because we want to understand how we declare the support 
> level of this UEFI UDF driver. If we just say we support UDF, then naive 
> people may believe we support everything. J
Yes :-)

> 
> 3)Which compatibility test has been done?
> 
> I am sorry that I cannot find the first version patch. I fund you 
> mentioned Win10 ISO is tried in V2. Any more?
> 
> We would like to know how many existing OS installation CDROM (or any 
> other CDROM) has been tried. Such as Linux (RedHat, Ubuntu, Suse, etc), 
> or Windows (Win7, Win8, Win10)?

Currently, I'm only using a Windows 10 Enterprise ISO image. But I also 
tested it with Windows 8 ISO images as well (in 3 years ago :-) )

Like I said, the Fedora ISO image I have didn't serve because it has no 
UDF file system. Wondering if any other Linux ISO image contains an UDF 
file system.

> 
> The more detail, the better. May a list.
> 
> 4)The last but not least important, which negative test (security test) 
> has been done?

None.

> 
> Have you run some fuzzing test?

No.

> 
> Have you tried a mal-format UDF disc? For example, with bad offset or 
> length?

No.

> 
> Have you test the integer overflow / buffer over flow handing in the code?

No.

> 
> NOTE: we should not use ASSERT to handle such error.
> 
> When I review the code, I found below:
> 
>      Status = ReadFileData (
> 
>        BlockIo,
> 
>        DiskIo,
> 
>        Volume,
> 
>        Parent,
> 
>        PrivFileData->FileSize,
> 
>        &PrivFileData->FilePosition,
> 
>        Buffer,
> 
>        &BufferSizeUint64
> 
>        );
> 
>      ASSERT (BufferSizeUint64 <= MAX_UINTN);
> 
>      *BufferSize = (UINTN)BufferSizeUint64;
> 
> I am not sure if we can use ASSERT (BufferSizeUint64 <= MAX_UINTN);
> 
> Can a malicious user construct a bad UDF and make BufferSizeUint64 > 
> MAX_UINTN?
> 
> Does it might happen? Or never happen?

Laszlo already answered :-)

> 
> We documented Appendix B - EDKII code review top 5 in 
> https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Security_Design_Guide_in_EDK_II.pdf
> 
> 3 of them are matched in these partition and file system drivers. I 
> quote below:
> 
> ===============================
> 
> *If the code consumes input from an untrusted source or region,*
> 
> Ensure that the input is rigorously and adequately validated.
> 
> *Verify buffer overflow is handled. Avoid integer overflow.*
> 
> Try to use subtraction instead of addition and division instead of 
> multiplication.
> 
> *Verify that ASSERT is used properly.*
> 
> ASSERT is used to catch conditions that should /never /happen. If 
> something /might /happen, use error handling instead. We can use both 
> ASSERT and error handling to facilitate debugging – ASSERT allows for 
> earlier detection and isolation of several classes of issues. [McConnell]
> 
> ===============================
> 
> It is just a reminder. If you are already following that, it will be 
> great. Please let us now.

No, I wasn't. But I will make sure to follow that rigorously next time. 
Very good info. Thanks!

> 
> I take a glance of UDF 2.6 specification, but I do not have chance to 
> read all content at this moment.
> 
> If I asked some stupid question , please feel free to correct me.

Not that I know of. I'm also not an UDF expert. I do enjoy writing 
read-only fs drivers in my free time, that is. :-)

> 
> All in all, we hope to understand the current quality level of the UDF 
> partition support and UDF file system.

Sure. You're completely right by asking such questions. If we really 
want to make proper use of it, the more understanding, testing, 
documentation, the better. Thank you very much for all the questions and 
suggestions. Really appreciate it.

I hope you guys (+ community) to test this implementation, report bugs, 
etc. as much as possible. I wish I could work on it full time and give 
better support for you guys. Unfortunately I can't. But I'll do my best 
in giving you some support.

Thanks!
Paulo

> 
> If you can help to provide the detail, it may help us to evaluate.
> 
> Thank you
> 
> Yao Jiewen
> 
> *From:*edk2-devel [mailto:edk2-devel-bounces@lists.01.org] *On Behalf Of 
> *Laszlo Ersek
> *Sent:* Sunday, September 17, 2017 6:17 AM
> *To:* Paulo Alcantara <pcacjr@zytor.com>
> *Cc:* Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; 
> edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Kinney, 
> Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>
> *Subject:* Re: [edk2] [PATCH 0/3] UDF partition driver fix
> 
> Hi Paulo,
> 
> On 09/16/17 23:36, Paulo Alcantara wrote:
>> This series fixes an UDF issue with Partition driver as discussed in ML:
>> 
>> https://lists.01.org/pipermail/edk2-devel/2017-September/014694.html
>> 
>> Thanks!
>> Paulo
>> 
>> Repo:   https://github.com/pcacjr/edk2.git
>> Branch: udf-partition-fix
>> 
>> Paulo Alcantara (3):
>>   MdePkg: Add UDF volume structure definitions
>>   MdeModulePkg/PartitionDxe: Fix creation of UDF logical partition
>>   MdeModulePkg/UdfDxe: Rework driver to support PartitionDxe changes
>> 
>>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c     | 307 +++++++++++-
>>  MdeModulePkg/Universal/Disk/UdfDxe/File.c          |  13 +-
>>  .../Universal/Disk/UdfDxe/FileSystemOperations.c   | 525 ++++++++-------------
>>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.c           |   7 -
>>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.h           |  88 +---
>>  MdePkg/Include/IndustryStandard/Udf.h              |  63 +++
>>  6 files changed, 560 insertions(+), 443 deletions(-)
>> 
> 
> Thank you very much for submitting this patchset quickly. I hope it will
> work out, and we won't need the PartitionExperimentalDxe.inf file!
> 
> I have some trivial process-level suggestions:
> 
> * when submitting a patchset, please collect the Cc: tags from all the
> commit messages, and add them to the cover letter manually. This way
> everybody you CC on at least some of the patches will get the cover
> letter too, presonally.
> 
> This matters because otherwise replies to the blurb will also miss those
> people, personally. (I'm now adding everyone manually.)
> 
> * Because edk2 uses long directory and file names, the diffstats are
> frequently truncated like above (see "..."). You can avoid this if you
> format the patches like this:
> 
>    --stat=1000 --stat-graph-width=20
> 
> this will make the pathname column just as wide as necessary, and will
> also keep the chart to the right reasonably narrow.
> 
> * It's probably best to include a reference to
> <https://bugzilla.tianocore.org/show_bug.cgi?id=707> in the commit
> messages (in particular patch #2).
> 
> * Once you post a patchset for a TianoCore BZ, it's useful to link the
> series (from the mailing list archive) in the BZ itself.
> 
> 
> Regarding the code itself, I don't think I can help here in any sensible
> way. (If UDF support were located under OvmfPkg, I would totally
> consider you the owner of those files, verify your patches for them on a
> formal level only, and if that part was fine, I'd give an Acked-by.)
> 
> Thanks!
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [PATCH 0/3] UDF partition driver fix

[Ni, Ruiyu]

Paulo,
I checked carefully of patch #1, 50% carefully of #2, 20% carefully of #3.
I could only give comments from the EDKII coding style perspective.
I do provide some other comments, but please understand they are from
a person that knows very little about UDF. (I know the concept of Volume.
But just know that there are so many different types of descriptors.)
If my understanding is wrong, please correct me!

Again, thanks for the contribution.

Thanks/Ray

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Paulo Alcantara
> Sent: Sunday, September 17, 2017 5:37 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH 0/3] UDF partition driver fix
> 
> This series fixes an UDF issue with Partition driver as discussed in ML:
> 
> https://lists.01.org/pipermail/edk2-devel/2017-September/014694.html
> 
> Thanks!
> Paulo
> 
> Repo:   https://github.com/pcacjr/edk2.git
> Branch: udf-partition-fix
> 
> Paulo Alcantara (3):
>   MdePkg: Add UDF volume structure definitions
>   MdeModulePkg/PartitionDxe: Fix creation of UDF logical partition
>   MdeModulePkg/UdfDxe: Rework driver to support PartitionDxe changes
> 
>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c     | 307 +++++++++++-
>  MdeModulePkg/Universal/Disk/UdfDxe/File.c          |  13 +-
>  .../Universal/Disk/UdfDxe/FileSystemOperations.c   | 525 ++++++++----------
> ---
>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.c           |   7 -
>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.h           |  88 +---
>  MdePkg/Include/IndustryStandard/Udf.h              |  63 +++
>  6 files changed, 560 insertions(+), 443 deletions(-)
> 
> --
> 2.11.0
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel