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

[Paulo Alcantara <pcacjr@zytor.com>]

Hi,

On 9/17/2017 9:54 PM, Ni, Ruiyu wrote:
> Paulo,
> Several comments:
> 1. Can below logic be removed in PartitionDxe/Udf.c?
> while (!IsDevicePathEnd (DevicePathNode)) {
>      //
>      // Do not allow checking for UDF file systems in CDROM "El Torito"
>      // partitions, and skip duplicate installation of UDF file system child
>      // nodes.
>      //
>      if (DevicePathType (DevicePathNode) == MEDIA_DEVICE_PATH) {
>        if (DevicePathSubType (DevicePathNode) == MEDIA_CDROM_DP) {
>          return EFI_NOT_FOUND;
>        }
>        if (DevicePathSubType (DevicePathNode) == MEDIA_VENDOR_DP) {
>          VendorDefinedGuid = (EFI_GUID *)((UINTN)DevicePathNode +
>                                           OFFSET_OF (VENDOR_DEVICE_PATH, Guid));
>          if (CompareGuid (VendorDefinedGuid, &UdfDevPathGuid)) {
>            return EFI_NOT_FOUND;
>          }
>        }
>      }
>      //
>      // Try next device path node
>      //
>      DevicePathNode = NextDevicePathNode (DevicePathNode);
>    }

I think it's no longer necessary, so I'll remove it.

> 
> 2. Can you add function header comments for the newly added functions?

Sure.

> 
> 3. Change the consuming code of IS_UDF_XXX macro after patch #1 is changed.

ACK.

> 
> 4. Perhaps to add more checks for these numbers read from the UDF CDROM.
> As Jiewen mentioned in another mail, secure code needs to validate all external inputs.
> We shouldn't assume the UDF CDROM is a well-formatted CDROM.
> A hacker may create a UDF CDROM to hack the system by using mechanism like
> buffer overflow, integer overflow, etc.

OK, when we start the Main Volume Descriptor Sequence (MVDS), we rely on 
"ExtentAd->ExtentLocation" to know the LBA
where to start the sequence, and on "ExtentAd->ExtentLength" to know 
many LBAs the sequence has. We only care about
LVD, PD and TD descriptors. We currently check whether a TD descriptor 
exists, or the search exceed ExtentAd->ExtentLength blocks
to determine when stopping the sequence. That doesn't seem to be safe 
enough, but here are some thoughts:

a. "ExtentAd->ExtentLength" may point to a bad value. If it's greater 
than or equal to BlockIo->Media->LastBlock, and there is no
TD descriptor, we'll have to walk the entire disk and there will too 
much I/O overhead and then impacting bootup. Obviously,
when it's greater than LastBlock, DiksIo->ReadDisk() should not allow to 
read a non-existent LBA and then return
EFI_INVALID_PARAMETER, as per UEFI spec.

b. "ExtentAd->ExtentLocation" might point to a invalid LBA, although 
DiskIo->ReadDisk() would not allow it.

So, it's time to look at UDF 2.60 spec again:

 > Volume Descriptor Sequence Extent
 >
 > Both the main and reserve volume descriptor sequence extents
 > shall each have a minimum length of 16 logical sectors. The VDS
 > Extent may be terminated by the extent length.

It doesn't tell us either a limit of logical blocks, or a maximum number.

Next:

 > 6.9.2.3 Step 3. Volume Descriptor Sequence
 > Read logical sectors:
 > MVDS_Location through MVDS_Location + (MVDS_Length - 1) / SectorSize
 > ...

MVDS_Length == ExtentAd->ExtentLength, so still insufficient.

Next:

 > The data located in bytes 0-3 and 5 of the Anchor Volume Descriptor 
Pointer may be
 > used for format verification if desired. Verifying the Tag Checksum 
in byte 4 and
 > Descriptor CRC in bytes 8-11 are good additional verifications to 
perform.
 > MVDS_Location and MVDS_Length are read from this structure.

Seems like a great check prior to reading ExtentAd->ExtentLocation and 
ExtentAd->ExtentLength contents. Would that be sufficient to you?

I haven't found a way to limit the number of blocks a Volume Descriptor 
Sequence might have.

Thanks,
Paulo

> 
> 
> Thanks/Ray
> 
>> -----Original Message-----
>> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>> Sent: Sunday, September 17, 2017 9:13 PM
>> To: edk2-devel@lists.01.org
>> Cc: Paulo Alcantara <pcacjr@zytor.com>; Dong, Eric <eric.dong@intel.com>;
>> Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Laszlo
>> Ersek <lersek@redhat.com>
>> Subject: [PATCH v2 2/3] MdeModulePkg/PartitionDxe: Fix creation of UDF
>> logical partition
>>
>> 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 | 323
>> ++++++++++++++++++--
>>   1 file changed, 299 insertions(+), 24 deletions(-)
>>
>> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
>> b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
>> index 7856b5dfc1..3e84882922 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,302 @@ 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 handled
>> +      //
>> +      if (LogicalVolDesc != NULL) {
>> +        Status = EFI_UNSUPPORTED;
>> +        goto Out_Free;
>> +      }
>> +
>> +      //
>> +      // 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 handled
>> +      //
>> +      if (PartitionDesc != NULL) {
>> +        Status = EFI_UNSUPPORTED;
>> +        goto Out_Free;
>> +      }
>> +
>> +      //
>> +      // 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++;
>> +  }
>> +
>> +  Status = EFI_VOLUME_CORRUPTED;
>> +
>> +  //
>> +  // 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
>> +    //
>> +    if (PartitionDesc->PartitionNumber != PartitionNum) {
>> +      Status = EFI_VOLUME_CORRUPTED;
>> +      goto Out_Free;
>> +    }
>> +
>> +    //
>> +    // 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
>> +    //
>> +    if (*LogicalVolEndLsn > LastBlock) {
>> +      Status = EFI_VOLUME_CORRUPTED;
>> +    } else {
>> +      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 +523,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 +566,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 +593,8 @@ PartitionInstallUdfChildHandles (
>>       DevicePath,
>>       (EFI_DEVICE_PATH_PROTOCOL *)&gUdfDevicePath,
>>       &PartitionInfo,
>> -    0,
>> -    Media->LastBlock,
>> +    StartingLBA,
>> +    EndingLBA,
>>       Media->BlockSize
>>       );
>>     if (!EFI_ERROR (Status)) {
>> --
>> 2.11.0
> 
>