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

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

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