[PATCH v2 0/2] Refine UDF related codes

[PATCH v2 0/2] Refine UDF related codes

[Hao Wu]

V2 changes:
For patch
MdeModulePkg/PartitionDxe: Merge the discovery of ElTorito into UDF:

PartitionInstallUdfChildHandles() should return EFI_SUCCESS when either El
Torito feature or UDF support is detected on CD/DVD media.

Previous version of the series only return EFI_SUCCESS when the UDF
support is detected, which may bring issue when only El Torito feature is
supported by a CD/DVD medium. For such case, child handle will be created
but the PartitionDxe driver will close the diskio & device path protocols
on the parent handle.


V1 history:
The series will do the following refinements:
a. Merge the discovery of the El Torito feature on CD/DVD media into the
   detect of UDF;
b. Avoid possible loss track of the allocated buffer pointer in UdfDxe.

Cc: Paulo Alcantara <pcacjr@zytor.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>

Hao Wu (2):
  MdeModulePkg/PartitionDxe: Merge the discovery of ElTorito into UDF
  MdeModulePkg/UdfDxe: Avoid possible loss track of allocated buffer

 MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c      |  7 +++--
 MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c            | 27 ++++++++++++++++++--
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 12 +++++----
 3 files changed, 35 insertions(+), 11 deletions(-)

-- 
2.12.0.windows.1

[PATCH v2 1/2] MdeModulePkg/PartitionDxe: Merge the discovery of ElTorito into UDF

[Hao Wu]

In order to create all of the children (El Torito standard and UDF) for
a CD/DVD media in an entry of the PartitionDriverBindingStart(), this
commit merges the discovery of the El Torito feature
(PartitionInstallElToritoChildHandles) into function
PartitionInstallUdfChildHandles.

Cc: Paulo Alcantara <pcacjr@zytor.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c |  7 +++--
 MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c       | 27 ++++++++++++++++++--
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
index f6030e0897..603abfe55a 100644
--- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
+++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
@@ -43,7 +43,6 @@ EFI_DRIVER_BINDING_PROTOCOL gPartitionDriverBinding = {
 //
 PARTITION_DETECT_ROUTINE mPartitionDetectRoutineTable[] = {
   PartitionInstallGptChildHandles,
-  PartitionInstallElToritoChildHandles,
   PartitionInstallMbrChildHandles,
   PartitionInstallUdfChildHandles,
   NULL
@@ -306,9 +305,9 @@ PartitionDriverBindingStart (
   if (BlockIo->Media->MediaPresent ||
       (BlockIo->Media->RemovableMedia && !BlockIo->Media->LogicalPartition)) {
     //
-    // Try for GPT, then El Torito, then UDF, and then legacy MBR partition
-    // types. If the media supports a given partition type install child handles
-    // to represent the partitions described by the media.
+    // Try for GPT, then legacy MBR partition types, and then UDF and El Torito.
+    // If the media supports a given partition type install child handles to
+    // represent the partitions described by the media.
     //
     Routine = &mPartitionDetectRoutineTable[0];
     while (*Routine != NULL) {
diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
index 7eee748958..5aac5640f6 100644
--- a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
+++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
@@ -688,8 +688,10 @@ PartitionInstallUdfChildHandles (
   EFI_PARTITION_INFO_PROTOCOL  PartitionInfo;
   EFI_LBA                      StartingLBA;
   EFI_LBA                      EndingLBA;
+  BOOLEAN                      ChildCreated;
 
   Media = BlockIo->Media;
+  ChildCreated = FALSE;
 
   //
   // Check if UDF logical block size is multiple of underlying device block size
@@ -704,11 +706,29 @@ PartitionInstallUdfChildHandles (
   }
 
   //
+  // Detect El Torito feature first.
+  // And always continue to search for UDF.
+  //
+  Status = PartitionInstallElToritoChildHandles (
+             This,
+             Handle,
+             DiskIo,
+             DiskIo2,
+             BlockIo,
+             BlockIo2,
+             DevicePath
+             );
+  if (!EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_INFO, "PartitionDxe: El Torito standard found on handle 0x%p.\n", Handle));
+    ChildCreated = TRUE;
+  }
+
+  //
   // Search for an UDF file system on block device
   //
   Status = FindUdfFileSystem (BlockIo, DiskIo, &StartingLBA, &EndingLBA);
   if (EFI_ERROR (Status)) {
-    return EFI_NOT_FOUND;
+    return (ChildCreated ? EFI_SUCCESS : EFI_NOT_FOUND);
   }
 
   //
@@ -735,6 +755,9 @@ PartitionInstallUdfChildHandles (
     EndingLBA,
     Media->BlockSize
     );
+  if (EFI_ERROR (Status)) {
+    return (ChildCreated ? EFI_SUCCESS : Status);
+  }
 
-  return Status;
+  return EFI_SUCCESS;
 }
-- 
2.12.0.windows.1

[PATCH v2 2/2] MdeModulePkg/UdfDxe: Avoid possible loss track of allocated buffer

[Hao Wu]

In function FindFileEntry():

Instead of using the function parameter 'FileEntry', use a local
variable to store the buffer allocated for disk read operation.

For the below calling stack:
UdfOpenVolume() -> FindRootDirectory() -> FindFileEntry()

In FindFileEntry(), the call to 'DiskIo->ReadDisk()' is possible (e.g.
media change for a CD/DVD ROM device) to trigger a re-install of the
BlockIO(2) protocol which will further lead to a call of the BindingStop()
& BingdingStart() of the UdfDxe driver.

Meanwhile, for the above listed calling stack, the '**FileEntry'
parameter passed into FindFileEntry() is '&PrivFsData->Root'. 'PrivFsData'
is a driver-managed private data, it will be freed in BindingStop() and
re-allocate in BingdingStart().

In such case, if '*FileEntry' is used to store the allocated buffer, the
information will be lost if 'DiskIo->ReadDisk()' triggers a re-install of
the BlockIO(2) protocol. The subsequent call of the FreePool API:

FreePool (*FileEntry);

will cause issues.

This commit uses a local variable to store the allocated buffer.

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <pcacjr@zytor.com>
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index e048d95d31..ecc172303e 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -1615,12 +1615,13 @@ FindFileEntry (
   UINT64              Lsn;
   UINT32              LogicalBlockSize;
   UDF_DESCRIPTOR_TAG  *DescriptorTag;
+  VOID                *ReadBuffer;
 
   Lsn               = GetLongAdLsn (Volume, Icb);
   LogicalBlockSize  = Volume->LogicalVolDesc.LogicalBlockSize;
 
-  *FileEntry = AllocateZeroPool (Volume->FileEntrySize);
-  if (*FileEntry == NULL) {
+  ReadBuffer = AllocateZeroPool (Volume->FileEntrySize);
+  if (ReadBuffer == NULL) {
     return EFI_OUT_OF_RESOURCES;
   }
 
@@ -1632,13 +1633,13 @@ FindFileEntry (
     BlockIo->Media->MediaId,
     MultU64x32 (Lsn, LogicalBlockSize),
     Volume->FileEntrySize,
-    *FileEntry
+    ReadBuffer
     );
   if (EFI_ERROR (Status)) {
     goto Error_Read_Disk_Blk;
   }
 
-  DescriptorTag = *FileEntry;
+  DescriptorTag = ReadBuffer;
 
   //
   // Check if the read extent contains a valid Tag Identifier for the expected
@@ -1650,11 +1651,12 @@ FindFileEntry (
     goto Error_Invalid_Fe;
   }
 
+  *FileEntry = ReadBuffer;
   return EFI_SUCCESS;
 
 Error_Invalid_Fe:
 Error_Read_Disk_Blk:
-  FreePool (*FileEntry);
+  FreePool (ReadBuffer);
 
   return Status;
 }
-- 
2.12.0.windows.1

Re: [PATCH v2 1/2] MdeModulePkg/PartitionDxe: Merge the discovery of ElTorito into UDF

[Paulo Alcantara]

On Thu, November 16, 2017 5:47 am, Hao Wu wrote:
> In order to create all of the children (El Torito standard and UDF) for
> a CD/DVD media in an entry of the PartitionDriverBindingStart(), this
> commit merges the discovery of the El Torito feature
> (PartitionInstallElToritoChildHandles) into function
> PartitionInstallUdfChildHandles.
>
> Cc: Paulo Alcantara <pcacjr@zytor.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Hao Wu <hao.a.wu@intel.com>
> ---
>  MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c |  7 +++--
>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c       | 27
> ++++++++++++++++++--
>  2 files changed, 28 insertions(+), 6 deletions(-)

Reviewed-by: Paulo Alcantara <pcacjr@zytor.com>

Thanks!
Paulo

>
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> index f6030e0897..603abfe55a 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
> @@ -43,7 +43,6 @@ EFI_DRIVER_BINDING_PROTOCOL gPartitionDriverBinding = {
>  //
>  PARTITION_DETECT_ROUTINE mPartitionDetectRoutineTable[] = {
>    PartitionInstallGptChildHandles,
> -  PartitionInstallElToritoChildHandles,
>    PartitionInstallMbrChildHandles,
>    PartitionInstallUdfChildHandles,
>    NULL
> @@ -306,9 +305,9 @@ PartitionDriverBindingStart (
>    if (BlockIo->Media->MediaPresent ||
>        (BlockIo->Media->RemovableMedia &&
> !BlockIo->Media->LogicalPartition)) {
>      //
> -    // Try for GPT, then El Torito, then UDF, and then legacy MBR
> partition
> -    // types. If the media supports a given partition type install child
> handles
> -    // to represent the partitions described by the media.
> +    // Try for GPT, then legacy MBR partition types, and then UDF and El
> Torito.
> +    // If the media supports a given partition type install child handles
> to
> +    // represent the partitions described by the media.
>      //
>      Routine = &mPartitionDetectRoutineTable[0];
>      while (*Routine != NULL) {
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> index 7eee748958..5aac5640f6 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> @@ -688,8 +688,10 @@ PartitionInstallUdfChildHandles (
>    EFI_PARTITION_INFO_PROTOCOL  PartitionInfo;
>    EFI_LBA                      StartingLBA;
>    EFI_LBA                      EndingLBA;
> +  BOOLEAN                      ChildCreated;
>
>    Media = BlockIo->Media;
> +  ChildCreated = FALSE;
>
>    //
>    // Check if UDF logical block size is multiple of underlying device
> block size
> @@ -704,11 +706,29 @@ PartitionInstallUdfChildHandles (
>    }
>
>    //
> +  // Detect El Torito feature first.
> +  // And always continue to search for UDF.
> +  //
> +  Status = PartitionInstallElToritoChildHandles (
> +             This,
> +             Handle,
> +             DiskIo,
> +             DiskIo2,
> +             BlockIo,
> +             BlockIo2,
> +             DevicePath
> +             );
> +  if (!EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_INFO, "PartitionDxe: El Torito standard found on handle
> 0x%p.\n", Handle));
> +    ChildCreated = TRUE;
> +  }
> +
> +  //
>    // Search for an UDF file system on block device
>    //
>    Status = FindUdfFileSystem (BlockIo, DiskIo, &StartingLBA, &EndingLBA);
>    if (EFI_ERROR (Status)) {
> -    return EFI_NOT_FOUND;
> +    return (ChildCreated ? EFI_SUCCESS : EFI_NOT_FOUND);
>    }
>
>    //
> @@ -735,6 +755,9 @@ PartitionInstallUdfChildHandles (
>      EndingLBA,
>      Media->BlockSize
>      );
> +  if (EFI_ERROR (Status)) {
> +    return (ChildCreated ? EFI_SUCCESS : Status);
> +  }
>
> -  return Status;
> +  return EFI_SUCCESS;
>  }
> --
> 2.12.0.windows.1
>
>


-- 
Paulo Alcantara, HP Inc.
Speaking for myself only.