Thanks/Ray
> -----Original Message----- > From: Paulo Alcantara [mailto:[email protected]] > Sent: Tuesday, September 19, 2017 12:38 AM > To: Ni, Ruiyu <[email protected]>; [email protected] > Cc: Dong, Eric <[email protected]>; Zeng, Star <[email protected]>; > Laszlo Ersek <[email protected]> > 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:[email protected]] > >> Sent: Sunday, September 17, 2017 9:13 PM > >> To: [email protected] > >> Cc: Paulo Alcantara <[email protected]>; Dong, Eric > >> <[email protected]>; Ni, Ruiyu <[email protected]>; Zeng, Star > >> <[email protected]>; Laszlo Ersek <[email protected]> > >> 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 <[email protected]> > >> Cc: Ruiyu Ni <[email protected]> > >> Cc: Star Zeng <[email protected]> > >> Cc: Laszlo Ersek <[email protected]> > >> Reported-by: Ruiyu Ni <[email protected]> > >> Contributed-under: TianoCore Contribution Agreement 1.1 > >> Signed-off-by: Paulo Alcantara <[email protected]> > >> --- > >> 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 [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
