From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (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 0B33920945B62 for ; Sun, 17 Sep 2017 17:51:07 -0700 (PDT) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Sep 2017 17:54:09 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,410,1500966000"; d="scan'208";a="1015427965" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga003.jf.intel.com with ESMTP; 17 Sep 2017 17:54:09 -0700 Received: from fmsmsx158.amr.corp.intel.com (10.18.116.75) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 17 Sep 2017 17:54:09 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx158.amr.corp.intel.com (10.18.116.75) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 17 Sep 2017 17:54:08 -0700 Received: from shsmsx151.ccr.corp.intel.com ([169.254.3.98]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.4]) with mapi id 14.03.0319.002; Mon, 18 Sep 2017 08:54:06 +0800 From: "Ni, Ruiyu" To: Paulo Alcantara , "edk2-devel@lists.01.org" CC: "Dong, Eric" , "Zeng, Star" , Laszlo Ersek Thread-Topic: [PATCH v2 2/3] MdeModulePkg/PartitionDxe: Fix creation of UDF logical partition Thread-Index: AQHTL7bdvEoQhbf3FUqqyg2jW1sWRKK5z7aw Date: Mon, 18 Sep 2017 00:54:05 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5BA44216@SHSMSX151.ccr.corp.intel.com> References: <1321928015f9ed919425051e0d75c132de77348c.1505653040.git.pcacjr@zytor.com> In-Reply-To: <1321928015f9ed919425051e0d75c132de77348c.1505653040.git.pcacjr@zytor.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 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 00:51:07 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Paulo, Several comments: 1. Can below logic be removed in PartitionDxe/Udf.c? =20 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) =3D=3D MEDIA_DEVICE_PATH) { if (DevicePathSubType (DevicePathNode) =3D=3D MEDIA_CDROM_DP) { return EFI_NOT_FOUND; } if (DevicePathSubType (DevicePathNode) =3D=3D MEDIA_VENDOR_DP) { VendorDefinedGuid =3D (EFI_GUID *)((UINTN)DevicePathNode + OFFSET_OF (VENDOR_DEVICE_PATH, Gui= d)); if (CompareGuid (VendorDefinedGuid, &UdfDevPathGuid)) { return EFI_NOT_FOUND; } } } // // Try next device path node // DevicePathNode =3D 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 exte= rnal 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 ; Dong, Eric ; > Ni, Ruiyu ; Zeng, Star ; Laszlo > Ersek > Subject: [PATCH v2 2/3] MdeModulePkg/PartitionDxe: Fix creation of UDF > logical partition >=20 > Do not use entire block device size for the UDF logical partition, instea= d > reserve the appropriate space (LVD space) for it. >=20 > 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(-) >=20 > 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; > } >=20 > -/** > - 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; >=20 > ZeroMem ((VOID *)&TerminatingVolDescriptor, sizeof > (CDROM_VOLUME_DESCRIPTOR)); >=20 > @@ -207,12 +190,302 @@ SupportUdfFileSystem ( > return EFI_UNSUPPORTED; > } >=20 > + return EFI_SUCCESS; > +} > + > +EFI_STATUS > +GetPartitionNumber ( > + IN UDF_LOGICAL_VOLUME_DESCRIPTOR *LogicalVolDesc, > + OUT UINT16 *PartitionNum > + ) > +{ > + EFI_STATUS Status; > + UDF_LONG_ALLOCATION_DESCRIPTOR *LongAd; > + > + Status =3D EFI_SUCCESS; > + > + switch (LV_UDF_REVISION (LogicalVolDesc)) { case 0x0102: > + // > + // UDF 1.20 only supports Type 1 Partition > + // > + *PartitionNum =3D *(UINT16 *)((UINTN)&LogicalVolDesc- > >PartitionMaps[4]); > + break; > + case 0x0150: > + // > + // Ensure Type 1 Partition map > + // > + ASSERT (LogicalVolDesc->PartitionMaps[0] =3D=3D 1 && > + LogicalVolDesc->PartitionMaps[1] =3D=3D 6); > + *PartitionNum =3D *(UINT16 *)((UINTN)&LogicalVolDesc- > >PartitionMaps[4]); > + break; > + case 0x0260: > + LongAd =3D &LogicalVolDesc->LogicalVolumeContentsUse; > + *PartitionNum =3D LongAd->ExtentLocation.PartitionReferenceNumber; > + break; > + default: > + // > + // Unhandled UDF revision > + // > + Status =3D 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 =3D BlockIo->Media->BlockSize; > + LastBlock =3D BlockIo->Media->LastBlock; > + ExtentAd =3D &AnchorPoint->MainVolumeDescriptorSequenceEx= tent; > + StartingLsn =3D (UINT64)ExtentAd->ExtentLocation; > + EndingLsn =3D > + StartingLsn + DivU64x32 ((UINT64)ExtentAd->ExtentLength, > + BlockSize); > + > + LogicalVolDesc =3D NULL; > + PartitionDesc =3D NULL; > + GuardMainVdsStartLsn =3D StartingLsn; > + > + // > + // Allocate buffer for reading disk blocks // Buffer =3D > + AllocateZeroPool (BlockSize); if (Buffer =3D=3D NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + Status =3D 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 <=3D EndingLsn) { > + // > + // Read disk block > + // > + Status =3D 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 !=3D NULL) { > + Status =3D 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 implemen= tation. > + // > + LogicalVolDesc =3D AllocateZeroPool (sizeof (*LogicalVolDesc)); > + if (LogicalVolDesc =3D=3D NULL) { > + Status =3D 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 !=3D NULL) { > + Status =3D 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 no= n- > + // 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 =3D AllocateZeroPool (sizeof (*PartitionDesc)); > + if (PartitionDesc =3D=3D NULL) { > + Status =3D EFI_OUT_OF_RESOURCES; > + goto Out_Free; > + } > + > + // > + // Save Partition Descriptor > + // > + CopyMem (PartitionDesc, Buffer, sizeof (*PartitionDesc)); > + } > + // > + // Go to next disk block > + // > + StartingLsn++; > + } > + > + Status =3D EFI_VOLUME_CORRUPTED; > + > + // > + // Check if LVD and PD were found > + // > + if (LogicalVolDesc !=3D NULL && PartitionDesc !=3D NULL) { > + // > + // Get partition number from Partition map in LVD descriptor > + // > + Status =3D GetPartitionNumber (LogicalVolDesc, &PartitionNum); > + if (EFI_ERROR (Status)) { > + goto Out_Free; > + } > + > + // > + // Make sure we're handling expected Partition Descriptor > + // > + if (PartitionDesc->PartitionNumber !=3D PartitionNum) { > + Status =3D 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 =3D GuardMainVdsStartLsn; > + *LogicalVolEndLsn =3D *MainVdsStartLsn + > + (UINT64)ExtentAd->ExtentLength; > + > + // > + // Cover UDF partition area > + // > + *LogicalVolEndLsn +=3D > + ((UINT64)PartitionDesc->PartitionStartingLocation - > + *LogicalVolEndLsn) + PartitionDesc->PartitionLength - 1; > + // > + // Ensure to not attempt reading past end of device > + // > + if (*LogicalVolEndLsn > LastBlock) { > + Status =3D EFI_VOLUME_CORRUPTED; > + } else { > + Status =3D EFI_SUCCESS; > + } > + } > + > +Out_Free: > + // > + // Free block read buffer > + // > + FreePool (Buffer); > + // > + // Free Logical Volume Descriptor > + // > + if (LogicalVolDesc !=3D NULL) { > + FreePool (LogicalVolDesc); > + } > + // > + // Free Partition Descriptor > + // > + if (PartitionDesc !=3D 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 =3D FindUdfVolumeIdentifiers (BlockIo, DiskIo); if (EFI_ERROR > + (Status)) { > + return Status; > + } > + > + // > + // Find Anchor Volume Descriptor Pointer // > Status =3D FindAnchorVolumeDescriptorPointer (BlockIo, DiskIo, > &AnchorPoint); > if (EFI_ERROR (Status)) { > - return EFI_UNSUPPORTED; > + return Status; > } >=20 > - return EFI_SUCCESS; > + // > + // Find Logical Volume location > + // > + Status =3D FindLogicalVolumeLocation ( > + BlockIo, > + DiskIo, > + &AnchorPoint, > + (UINT64 *)StartingLBA, > + (UINT64 *)EndingLBA > + ); > + > + return Status; > } >=20 > /** > @@ -250,6 +523,8 @@ PartitionInstallUdfChildHandles ( > EFI_GUID *VendorDefinedGuid; > EFI_GUID UdfDevPathGuid =3D EFI_UDF_DEVICE_PATH_GU= ID; > EFI_PARTITION_INFO_PROTOCOL PartitionInfo; > + EFI_LBA StartingLBA; > + EFI_LBA EndingLBA; >=20 > Media =3D BlockIo->Media; >=20 > @@ -291,9 +566,9 @@ PartitionInstallUdfChildHandles ( > } >=20 > // > - // Check if block device supports an UDF file system > + // Find UDF logical volume on block device > // > - Status =3D SupportUdfFileSystem (BlockIo, DiskIo); > + Status =3D 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