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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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);
}
2. Can you add function header comments for the newly added functions?
3. Change the consuming code of IS_UDF_XXX macro after patch #1 is changed.
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.
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
> >
> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ray, On 09/20/17 10:14, Ni, Ruiyu wrote: > 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. side question: do you mean "PatchCheck.py"? I've never heard of "Ecc.py" before, what does it do? (The top comment only says "This file is used to be the main entrance of ECC tool", and the "BaseTools/UserManuals" directory doesn't seem to contain anything related.) Thanks, Laslzo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
I am surprised that you don't know ECC tool. 😊 It's a much more complex tool than PatchCheck.py. It's to report all coding style issues that doesn't follow EDKII C coding style. Thanks/Ray > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Wednesday, September 20, 2017 5:59 PM > To: Ni, Ruiyu <ruiyu.ni@intel.com>; Paulo Alcantara <pcacjr@zytor.com>; > edk2-devel@lists.01.org > Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: Re: [PATCH v2 2/3] MdeModulePkg/PartitionDxe: Fix creation of > UDF logical partition > > Ray, > > On 09/20/17 10:14, Ni, Ruiyu wrote: > > > 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. > > side question: do you mean "PatchCheck.py"? > > I've never heard of "Ecc.py" before, what does it do? (The top comment only > says "This file is used to be the main entrance of ECC tool", and the > "BaseTools/UserManuals" directory doesn't seem to contain anything > related.) > > Thanks, > Laslzo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 09/20/17 12:11, Ni, Ruiyu wrote:
> I am surprised that you don't know ECC tool. 😊
> It's a much more complex tool than PatchCheck.py.
> It's to report all coding style issues that doesn't follow EDKII C coding style.
Well, before my previous email, I tried to execute the command line you named, but it didn't work:
$ source edksetup.sh
$ Ecc -t MdeModulePkg/Universal/Disk/PartitionDxe -s
Traceback (most recent call last):
File "BaseTools/BinWrappers/PosixLike/../../Source/Python/Ecc/Ecc.py", line 24, in <module>
from Check import Check
File "BaseTools/Source/Python/Ecc/Check.py", line 20, in <module>
import c
File "BaseTools/Source/Python/Ecc/c.py", line 18, in <module>
import CodeFragmentCollector
File "BaseTools/Source/Python/Ecc/CodeFragmentCollector.py", line 23, in <module>
import antlr3
ImportError: No module named antlr3
Now I'v checked both Fedora and RHEL7 packages for an "antlr3" python module, but it doesn't exist. The "antlr3" package itself exists in both distros, but the changelogs say,
> * Thu Feb 23 2012 Miloš Jakubíček <xjakub@fi.muni.cz>
> - 3.4-5 - Disable python runtime (incompatible with current antlr version)
The following RHBZ looks relevant: <https://bugzilla.redhat.com/show_bug.cgi?id=1313024>
Either way, I don't think I can use the ECC tool.
Thanks,
Laszlo
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, September 20, 2017 5:59 PM
>> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Paulo Alcantara <pcacjr@zytor.com>;
>> edk2-devel@lists.01.org
>> Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
>> Subject: Re: [PATCH v2 2/3] MdeModulePkg/PartitionDxe: Fix creation of
>> UDF logical partition
>>
>> Ray,
>>
>> On 09/20/17 10:14, Ni, Ruiyu wrote:
>>
>>> 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.
>>
>> side question: do you mean "PatchCheck.py"?
>>
>> I've never heard of "Ecc.py" before, what does it do? (The top comment only
>> says "This file is used to be the main entrance of ECC tool", and the
>> "BaseTools/UserManuals" directory doesn't seem to contain anything
>> related.)
>>
>> Thanks,
>> Laslzo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo: ECC depends on antlr V3.0.1. It can be downloaded from http://www.antlr3.org/download/Python/ After download it, use python setup.py install to install it. I try this way on Windows OS, it can work. Thanks Liming > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek > Sent: Wednesday, September 20, 2017 7:10 PM > To: Ni, Ruiyu <ruiyu.ni@intel.com>; Paulo Alcantara <pcacjr@zytor.com>; edk2-devel@lists.01.org > Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: Re: [edk2] [PATCH v2 2/3] MdeModulePkg/PartitionDxe: Fix creation of UDF logical partition > > On 09/20/17 12:11, Ni, Ruiyu wrote: > > I am surprised that you don't know ECC tool. 😊 > > It's a much more complex tool than PatchCheck.py. > > It's to report all coding style issues that doesn't follow EDKII C coding style. > > Well, before my previous email, I tried to execute the command line you named, but it didn't work: > > $ source edksetup.sh > > $ Ecc -t MdeModulePkg/Universal/Disk/PartitionDxe -s > Traceback (most recent call last): > File "BaseTools/BinWrappers/PosixLike/../../Source/Python/Ecc/Ecc.py", line 24, in <module> > from Check import Check > File "BaseTools/Source/Python/Ecc/Check.py", line 20, in <module> > import c > File "BaseTools/Source/Python/Ecc/c.py", line 18, in <module> > import CodeFragmentCollector > File "BaseTools/Source/Python/Ecc/CodeFragmentCollector.py", line 23, in <module> > import antlr3 > ImportError: No module named antlr3 > > Now I'v checked both Fedora and RHEL7 packages for an "antlr3" python module, but it doesn't exist. The "antlr3" package itself exists > in both distros, but the changelogs say, > > > * Thu Feb 23 2012 Miloš Jakubíček <xjakub@fi.muni.cz> > > - 3.4-5 - Disable python runtime (incompatible with current antlr version) > > The following RHBZ looks relevant: <https://bugzilla.redhat.com/show_bug.cgi?id=1313024> > > Either way, I don't think I can use the ECC tool. > > Thanks, > Laszlo > > > >> -----Original Message----- > >> From: Laszlo Ersek [mailto:lersek@redhat.com] > >> Sent: Wednesday, September 20, 2017 5:59 PM > >> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Paulo Alcantara <pcacjr@zytor.com>; > >> edk2-devel@lists.01.org > >> Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com> > >> Subject: Re: [PATCH v2 2/3] MdeModulePkg/PartitionDxe: Fix creation of > >> UDF logical partition > >> > >> Ray, > >> > >> On 09/20/17 10:14, Ni, Ruiyu wrote: > >> > >>> 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. > >> > >> side question: do you mean "PatchCheck.py"? > >> > >> I've never heard of "Ecc.py" before, what does it do? (The top comment only > >> says "This file is used to be the main entrance of ECC tool", and the > >> "BaseTools/UserManuals" directory doesn't seem to contain anything > >> related.) > >> > >> Thanks, > >> Laslzo > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.