From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.zytor.com (terminus.zytor.com [65.50.211.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 509CF21D046B1 for ; Mon, 18 Sep 2017 09:37:44 -0700 (PDT) Received: from [10.26.0.110] (corporativo.static.gvt.net.br [177.135.97.54] (may be forged)) (authenticated bits=0) by mail.zytor.com (8.15.2/8.15.2) with ESMTPSA id v8IGcVRO010146 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Mon, 18 Sep 2017 09:38:33 -0700 To: "Ni, Ruiyu" , "edk2-devel@lists.01.org" Cc: "Dong, Eric" , "Zeng, Star" , Laszlo Ersek References: <1321928015f9ed919425051e0d75c132de77348c.1505653040.git.pcacjr@zytor.com> <734D49CCEBEEF84792F5B80ED585239D5BA44216@SHSMSX151.ccr.corp.intel.com> From: Paulo Alcantara Message-ID: <71ad2dca-6b6a-f6f7-cf4b-1018f7682306@zytor.com> Date: Mon, 18 Sep 2017 13:38:25 -0300 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5BA44216@SHSMSX151.ccr.corp.intel.com> Subject: Re: [PATCH v2 2/3] MdeModulePkg/PartitionDxe: Fix creation of UDF logical partition X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Sep 2017 16:37:44 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 ; Dong, Eric ; >> Ni, Ruiyu ; Zeng, Star ; Laszlo >> Ersek >> 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 >> Cc: Ruiyu Ni >> Cc: Star Zeng >> Cc: Laszlo Ersek >> Reported-by: Ruiyu Ni >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Paulo Alcantara >> --- >> 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 > >