Re: [PATCH] MdeModulePkg/PartitionDxe: Fix UDF fs access on certain CD/DVD medias

["Wu, Hao A" <hao.a.wu@intel.com>]

Pushed at: 1fbe8276c4

Best Regards,
Hao Wu


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wu,
> Hao A
> Sent: Tuesday, November 14, 2017 3:49 PM
> To: 'Paulo Alcantara'; Ni, Ruiyu; edk2-devel@lists.01.org
> Cc: Dong, Eric; Zeng, Star
> Subject: Re: [edk2] [PATCH] MdeModulePkg/PartitionDxe: Fix UDF fs access on
> certain CD/DVD medias
> 
> Hi Paulo,
> 
> Sorry for the delayed response. The patch is good to me:
> Reviewed-by: Hao Wu <hao.a.wu@intel.com>
> 
> Hi Ray,
> 
> Do you have any other comment on this?
> 
> 
> Best Regards,
> Hao Wu
> 
> 
> > -----Original Message-----
> > From: Paulo Alcantara [mailto:pcacjr@zytor.com]
> > Sent: Friday, October 13, 2017 9:25 PM
> > To: edk2-devel@lists.01.org
> > Cc: Paulo Alcantara; Wu, Hao A; Ni, Ruiyu; Zeng, Star; Dong, Eric
> > Subject: [PATCH] MdeModulePkg/PartitionDxe: Fix UDF fs access on certain
> > CD/DVD medias
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=725
> >
> > Historically many drives or medias do not correctly return their value
> > for block N - which is also referred as last addressable/recorded block.
> > When they do so, there is still a problem when relying on last recorded
> > block number returned by SCSI commands READ CAPACITY and READ TRACK
> > INFORMATION - that is, between block 0 and block N there may be
> > unwritten blocks which are located outside any track.
> >
> > That said, the Partition driver was unable to find AVDP at block N on
> > certain medias that do not either return or report their last recorded
> > block number correctly.
> >
> > Apparently there is no official or correct way to find the correct block
> > number, however tools like the Philips UDF Conformance Tool (udf_test)
> > apply a correction by searching for an AVDP or VAT in blocks N through
> > N-456 -- this can be observed by looking at the log reported by udf_test
> > on those CD/DVD medias. So, if the AVDP or VAT is found, then it sets
> > the last recorded block number to where AVDP or VAT was located.
> >
> > With the below setence in UDF 2.60, 6.13.2.2 Background Physical
> > Formatting:
> >
> > "... the second AVDP must be recorded after the Background physical
> > Formatting has been finished..."
> >
> > Implies that the last recorded block is the one where second AVDP was
> > recorded.
> >
> > This patch implements a similar way to correct the last recorded block
> > number by searching for last AVDP in blocks N-1 through N-512 on those
> > certain medias, as well as ensure a minimum number of 2 AVDPs found as
> > specified by ECMA 167 and UDF 2.60 specifications.
> >
> > Cc: Hao Wu <hao.a.wu@intel.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Reported-by: Hao Wu <hao.a.wu@intel.com>
> > Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
> > ---
> >  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 254
> ++++++++++++++++--
> > --
> >  1 file changed, 201 insertions(+), 53 deletions(-)
> >
> > diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> > b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> > index 8aee30c759..7eee748958 100644
> > --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> > +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> > @@ -14,6 +14,8 @@
> >
> >  #include "Partition.h"
> >
> > +#define MAX_CORRECTION_BLOCKS_NUM 512u
> > +
> >  //
> >  // C5BD4D42-1A76-4996-8956-73CDA326CD0A
> >  //
> > @@ -48,61 +50,199 @@ UDF_DEVICE_PATH gUdfDevicePath = {
> >  /**
> >    Find the anchor volume descriptor pointer.
> >
> > -  @param[in]  BlockIo             BlockIo interface.
> > -  @param[in]  DiskIo              DiskIo interface.
> > -  @param[out] AnchorPoint         Anchor volume descriptor pointer.
> > +  @param[in]  BlockIo               BlockIo interface.
> > +  @param[in]  DiskIo                DiskIo interface.
> > +  @param[out] AnchorPoint           Anchor volume descriptor pointer.
> > +  @param[out] LastRecordedBlock     Last recorded block.
> >
> > -  @retval EFI_SUCCESS             Anchor volume descriptor pointer found.
> > -  @retval EFI_VOLUME_CORRUPTED    The file system structures are
> > corrupted.
> > -  @retval other                   Anchor volume descriptor pointer not found.
> > +  @retval EFI_SUCCESS               Anchor volume descriptor pointer found.
> > +  @retval EFI_VOLUME_CORRUPTED      The file system structures are
> > corrupted.
> > +  @retval other                     Anchor volume descriptor pointer not found.
> >
> >  **/
> >  EFI_STATUS
> >  FindAnchorVolumeDescriptorPointer (
> >    IN   EFI_BLOCK_IO_PROTOCOL                 *BlockIo,
> >    IN   EFI_DISK_IO_PROTOCOL                  *DiskIo,
> > -  OUT  UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER  *AnchorPoint
> > +  OUT  UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER  *AnchorPoint,
> > +  OUT  EFI_LBA                               *LastRecordedBlock
> >    )
> >  {
> > -  EFI_STATUS          Status;
> > -  UINT32              BlockSize;
> > -  EFI_LBA             EndLBA;
> > -  EFI_LBA             DescriptorLBAs[4];
> > -  UINTN               Index;
> > -  UDF_DESCRIPTOR_TAG  *DescriptorTag;
> > +  EFI_STATUS                            Status;
> > +  UINT32                                BlockSize;
> > +  EFI_LBA                               EndLBA;
> > +  UDF_DESCRIPTOR_TAG                    *DescriptorTag;
> > +  UINTN                                 AvdpsCount;
> > +  UINTN                                 Size;
> > +  UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER  *AnchorPoints;
> > +  INTN                                  Index;
> > +  UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER  *AnchorPointPtr;
> > +  EFI_LBA                               LastAvdpBlockNum;
> >
> > +  //
> > +  // UDF 2.60, 2.2.3 Anchor Volume Descriptor Pointer
> > +  //
> > +  // An Anchor Volume Descriptor Pointer structure shall be recorded in at
> > +  // least 2 of the following 3 locations on the media: Logical Sector 256,
> > +  // N - 256 or N, where N is the last *addressable* sector of a volume.
> > +  //
> > +  // To figure out what logical sector N is, the SCSI commands READ
> CAPACITY
> > and
> > +  // READ TRACK INFORMATION are used, however many drives or medias
> > report their
> > +  // "last recorded block" wrongly. Although, READ CAPACITY returns the last
> > +  // readable data block but there might be unwritten blocks, which are
> > located
> > +  // outside any track and therefore AVDP will not be found at block N.
> > +  //
> > +  // That said, we define a magic number of 512 blocks to be used as
> > correction
> > +  // when attempting to find AVDP and define last block number.
> > +  //
> >    BlockSize = BlockIo->Media->BlockSize;
> >    EndLBA = BlockIo->Media->LastBlock;
> > -  DescriptorLBAs[0] = 256;
> > -  DescriptorLBAs[1] = EndLBA - 256;
> > -  DescriptorLBAs[2] = EndLBA;
> > -  DescriptorLBAs[3] = 512;
> > +  *LastRecordedBlock = EndLBA;
> > +  AvdpsCount = 0;
> >
> > -  for (Index = 0; Index < ARRAY_SIZE (DescriptorLBAs); Index++) {
> > -    Status = DiskIo->ReadDisk (
> > -      DiskIo,
> > -      BlockIo->Media->MediaId,
> > -      MultU64x32 (DescriptorLBAs[Index], BlockSize),
> > -      sizeof (UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER),
> > -      (VOID *)AnchorPoint
> > -      );
> > -    if (EFI_ERROR (Status)) {
> > -      return Status;
> > -    }
> > +  //
> > +  // Find AVDP at block 256
> > +  //
> > +  Status = DiskIo->ReadDisk (
> > +    DiskIo,
> > +    BlockIo->Media->MediaId,
> > +    MultU64x32 (256, BlockSize),
> > +    sizeof (*AnchorPoint),
> > +    AnchorPoint
> > +    );
> > +  if (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> > +
> > +  DescriptorTag = &AnchorPoint->DescriptorTag;
> > +
> > +  //
> > +  // Check if read block is a valid AVDP descriptor
> > +  //
> > +  if (DescriptorTag->TagIdentifier == UdfAnchorVolumeDescriptorPointer) {
> > +    DEBUG ((DEBUG_INFO, "%a: found AVDP at block %d\n", __FUNCTION__,
> > 256));
> > +    AvdpsCount++;
> > +  }
> > +
> > +  //
> > +  // Find AVDP at block N - 256
> > +  //
> > +  Status = DiskIo->ReadDisk (
> > +    DiskIo,
> > +    BlockIo->Media->MediaId,
> > +    MultU64x32 ((UINT64)EndLBA - 256, BlockSize),
> > +    sizeof (*AnchorPoint),
> > +    AnchorPoint
> > +    );
> > +  if (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> > +
> > +  //
> > +  // Check if read block is a valid AVDP descriptor
> > +  //
> > +  if (DescriptorTag->TagIdentifier == UdfAnchorVolumeDescriptorPointer &&
> > +      ++AvdpsCount == 2) {
> > +    DEBUG ((DEBUG_INFO, "%a: found AVDP at block %Ld\n", __FUNCTION__,
> > +            EndLBA - 256));
> > +    return EFI_SUCCESS;
> > +  }
> > +
> > +  //
> > +  // Check if at least one AVDP was found in previous locations
> > +  //
> > +  if (AvdpsCount == 0) {
> > +    return EFI_VOLUME_CORRUPTED;
> > +  }
> > +
> > +  //
> > +  // Find AVDP at block N
> > +  //
> > +  Status = DiskIo->ReadDisk (
> > +    DiskIo,
> > +    BlockIo->Media->MediaId,
> > +    MultU64x32 ((UINT64)EndLBA, BlockSize),
> > +    sizeof (*AnchorPoint),
> > +    AnchorPoint
> > +    );
> > +  if (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> > +
> > +  //
> > +  // Check if read block is a valid AVDP descriptor
> > +  //
> > +  if (DescriptorTag->TagIdentifier == UdfAnchorVolumeDescriptorPointer) {
> > +    return EFI_SUCCESS;
> > +  }
> > +
> > +  //
> > +  // No AVDP found at block N. Possibly drive/media returned bad last
> > recorded
> > +  // block, or it is part of unwritten data blocks and outside any track.
> > +  //
> > +  // Search backwards for an AVDP from block N-1 through
> > +  // N-MAX_CORRECTION_BLOCKS_NUM. If any AVDP is found, then correct
> > last block
> > +  // number for the new UDF partition child handle.
> > +  //
> > +  Size = MAX_CORRECTION_BLOCKS_NUM * BlockSize;
> > +
> > +  AnchorPoints = AllocateZeroPool (Size);
> > +  if (AnchorPoints == NULL) {
> > +    return EFI_OUT_OF_RESOURCES;
> > +  }
> > +
> > +  //
> > +  // Read consecutive MAX_CORRECTION_BLOCKS_NUM disk blocks
> > +  //
> > +  Status = DiskIo->ReadDisk (
> > +    DiskIo,
> > +    BlockIo->Media->MediaId,
> > +    MultU64x32 ((UINT64)EndLBA - MAX_CORRECTION_BLOCKS_NUM,
> > BlockSize),
> > +    Size,
> > +    AnchorPoints
> > +    );
> > +  if (EFI_ERROR (Status)) {
> > +    goto Out_Free;
> > +  }
> >
> > -    DescriptorTag = &AnchorPoint->DescriptorTag;
> > +  Status = EFI_VOLUME_CORRUPTED;
> > +
> > +  //
> > +  // Search for AVDP from blocks N-1 through N-
> > MAX_CORRECTION_BLOCKS_NUM
> > +  //
> > +  for (Index = MAX_CORRECTION_BLOCKS_NUM - 2; Index >= 0; Index--) {
> > +    AnchorPointPtr = (VOID *)((UINTN)AnchorPoints + Index * BlockSize);
> > +
> > +    DescriptorTag = &AnchorPointPtr->DescriptorTag;
> >
> >      //
> > -    // Check if read LBA has a valid AVDP descriptor.
> > +    // Check if read block is a valid AVDP descriptor
> >      //
> >      if (DescriptorTag->TagIdentifier == UdfAnchorVolumeDescriptorPointer) {
> > -      return EFI_SUCCESS;
> > +      //
> > +      // Calculate last recorded block number
> > +      //
> > +      LastAvdpBlockNum = EndLBA - (MAX_CORRECTION_BLOCKS_NUM -
> > Index);
> > +      DEBUG ((DEBUG_WARN, "%a: found AVDP at block %Ld\n",
> > __FUNCTION__,
> > +              LastAvdpBlockNum));
> > +      DEBUG ((DEBUG_WARN, "%a: correcting last block from %Ld to %Ld\n",
> > +              __FUNCTION__, EndLBA, LastAvdpBlockNum));
> > +      //
> > +      // Save read AVDP from last block
> > +      //
> > +      CopyMem (AnchorPoint, AnchorPointPtr, sizeof (*AnchorPointPtr));
> > +      //
> > +      // Set last recorded block number
> > +      //
> > +      *LastRecordedBlock = LastAvdpBlockNum;
> > +      Status = EFI_SUCCESS;
> > +      break;
> >      }
> >    }
> > -  //
> > -  // No AVDP found.
> > -  //
> > -  return EFI_VOLUME_CORRUPTED;
> > +
> > +Out_Free:
> > +  FreePool (AnchorPoints);
> > +  return Status;
> >  }
> >
> >  /**
> > @@ -280,16 +420,17 @@ IsLogicalVolumeDescriptorSupported (
> >    Find UDF logical volume location and whether it is supported by current
> EDK2
> >    UDF file system implementation.
> >
> > -  @param[in]  BlockIo             BlockIo interface.
> > -  @param[in]  DiskIo              DiskIo interface.
> > -  @param[in]  AnchorPoint         Anchor volume descriptor pointer.
> > -  @param[out] MainVdsStartBlock   Main VDS starting block number.
> > -  @param[out] MainVdsEndBlock     Main VDS ending block number.
> > +  @param[in]  BlockIo               BlockIo interface.
> > +  @param[in]  DiskIo                DiskIo interface.
> > +  @param[in]  AnchorPoint           Anchor volume descriptor pointer.
> > +  @param[in]  LastRecordedBlock     Last recorded block in media.
> > +  @param[out] MainVdsStartBlock     Main VDS starting block number.
> > +  @param[out] MainVdsEndBlock       Main VDS ending block number.
> >
> > -  @retval EFI_SUCCESS             UDF logical volume was found.
> > -  @retval EFI_VOLUME_CORRUPTED    UDF file system structures are
> > corrupted.
> > -  @retval EFI_UNSUPPORTED         UDF logical volume is not supported.
> > -  @retval other                   Failed to perform disk I/O.
> > +  @retval EFI_SUCCESS               UDF logical volume was found.
> > +  @retval EFI_VOLUME_CORRUPTED      UDF file system structures are
> > corrupted.
> > +  @retval EFI_UNSUPPORTED           UDF logical volume is not supported.
> > +  @retval other                     Failed to perform disk I/O.
> >
> >  **/
> >  EFI_STATUS
> > @@ -297,13 +438,13 @@ FindLogicalVolumeLocation (
> >    IN   EFI_BLOCK_IO_PROTOCOL                 *BlockIo,
> >    IN   EFI_DISK_IO_PROTOCOL                  *DiskIo,
> >    IN   UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER  *AnchorPoint,
> > +  IN   EFI_LBA                               LastRecordedBlock,
> >    OUT  UINT64                                *MainVdsStartBlock,
> >    OUT  UINT64                                *MainVdsEndBlock
> >    )
> >  {
> >    EFI_STATUS                     Status;
> >    UINT32                         BlockSize;
> > -  EFI_LBA                        LastBlock;
> >    UDF_EXTENT_AD                  *ExtentAd;
> >    UINT64                         SeqBlocksNum;
> >    UINT64                         SeqStartBlock;
> > @@ -316,7 +457,6 @@ FindLogicalVolumeLocation (
> >    UDF_DESCRIPTOR_TAG             *DescriptorTag;
> >
> >    BlockSize = BlockIo->Media->BlockSize;
> > -  LastBlock = BlockIo->Media->LastBlock;
> >    ExtentAd = &AnchorPoint->MainVolumeDescriptorSequenceExtent;
> >
> >    //
> > @@ -328,7 +468,7 @@ FindLogicalVolumeLocation (
> >    // Also make sure it does not exceed maximum number of blocks in the disk.
> >    //
> >    SeqBlocksNum = DivU64x32 ((UINT64)ExtentAd->ExtentLength, BlockSize);
> > -  if (SeqBlocksNum < 16 || (EFI_LBA)SeqBlocksNum > LastBlock + 1) {
> > +  if (SeqBlocksNum < 16 || (EFI_LBA)SeqBlocksNum > LastRecordedBlock + 1)
> {
> >      return EFI_VOLUME_CORRUPTED;
> >    }
> >
> > @@ -336,8 +476,8 @@ FindLogicalVolumeLocation (
> >    // Check for valid Volume Descriptor Sequence starting block number
> >    //
> >    SeqStartBlock = (UINT64)ExtentAd->ExtentLocation;
> > -  if (SeqStartBlock > LastBlock ||
> > -      SeqStartBlock + SeqBlocksNum - 1 > LastBlock) {
> > +  if (SeqStartBlock > LastRecordedBlock ||
> > +      SeqStartBlock + SeqBlocksNum - 1 > LastRecordedBlock) {
> >      return EFI_VOLUME_CORRUPTED;
> >    }
> >
> > @@ -437,9 +577,10 @@ FindLogicalVolumeLocation (
> >      *MainVdsStartBlock = GuardMainVdsStartBlock;
> >      //
> >      // We do not need to read either LVD or PD descriptors to know the last
> > -    // valid block in the found UDF file system. It's already LastBlock.
> > +    // valid block in the found UDF file system. It's already
> > +    // LastRecordedBlock.
> >      //
> > -    *MainVdsEndBlock = LastBlock;
> > +    *MainVdsEndBlock = LastRecordedBlock;
> >
> >      Status = EFI_SUCCESS;
> >    }
> > @@ -473,8 +614,9 @@ FindUdfFileSystem (
> >    OUT EFI_LBA               *EndingLBA
> >    )
> >  {
> > -  EFI_STATUS Status;
> > +  EFI_STATUS                            Status;
> >    UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER  AnchorPoint;
> > +  EFI_LBA                               LastRecordedBlock;
> >
> >    //
> >    // Find UDF volume identifiers
> > @@ -487,7 +629,12 @@ FindUdfFileSystem (
> >    //
> >    // Find Anchor Volume Descriptor Pointer
> >    //
> > -  Status = FindAnchorVolumeDescriptorPointer (BlockIo, DiskIo,
> &AnchorPoint);
> > +  Status = FindAnchorVolumeDescriptorPointer (
> > +    BlockIo,
> > +    DiskIo,
> > +    &AnchorPoint,
> > +    &LastRecordedBlock
> > +    );
> >    if (EFI_ERROR (Status)) {
> >      return Status;
> >    }
> > @@ -499,6 +646,7 @@ FindUdfFileSystem (
> >      BlockIo,
> >      DiskIo,
> >      &AnchorPoint,
> > +    LastRecordedBlock,
> >      (UINT64 *)StartingLBA,
> >      (UINT64 *)EndingLBA
> >      );
> > --
> > 2.11.0
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel