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

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

Thanks/Ray

> -----Original Message-----
> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
> Sent: Tuesday, September 19, 2017 12:38 AM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>;
> Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [PATCH v2 2/3] MdeModulePkg/PartitionDxe: Fix creation of
> UDF logical partition
> 
> 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.

When you add the header comments, please run
Python BaseTools/Source/Python/Ecc/Ecc.py -t MdeModulePkg/Universal/Disk/PartitionDxe -s
To make sure there is no other coding style issue.

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

I think you may also need to declare the Buffer as UDF_DESCRIPTOR_TAG.

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


OK to me.

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