Re: [PATCH] MdeModulePkg/UdfDxe: Additional checks for ResolveSymlink()

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: Zeng, Star
> Sent: Friday, October 26, 2018 10:14 PM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu; Paulo Alcantara; Zeng, Star
> Subject: Re: [edk2] [PATCH] MdeModulePkg/UdfDxe: Additional checks for
> ResolveSymlink()
> 
> Hao,
> 
> On 2018/10/26 15:54, Hao Wu wrote:
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1279
> >
> > The commit will add 3 types of checks for function ResolveSymlink():
> >
> > A. Check for the value of 'Component Type' field within a Path Component
> >
> > According to the ECMA-167 standard (3rd Edition - June 1997), Section
> > 14.16.1.1, valid values are 1 to 5. All other values will be treated as a
> > corrupted volume.
> >
> > B. Check for the content pointed by 'File'
> >
> > Since content within 'File' is the output data for ResolveSymlink().
> > Checks is added to ensure the content in 'File' is valid. Otherwise,
> > possible null pointer dereference issue will occur during the subsequent
> > usage of the data returned by ResolveSymlink().
> >
> > C. Check for possible memory double free/use after free case
> >
> > For codes:
> >
> >      if (CompareMem ((VOID *)&PreviousFile, (VOID *)Parent,
> >                      sizeof (UDF_FILE_INFO)) != 0) {
> >        CleanupFileInformation (&PreviousFile);
> >      }
> >
> >      CopyMem ((VOID *)&PreviousFile, (VOID *)File, sizeof (UDF_FILE_INFO));
> >
> > If the contents in 'PreviousFile' and 'File' are the same, call to
> > "CleanupFileInformation (&PreviousFile);" will free the buffers in 'File'
> > as well. This will lead to potential memory double free/use after free
> > issues.
> 
> If 'PreviousFile' and 'File' are the same, the coping operation below
> also has no need to be done, right?

Yes.
I will send a V2 patch to address this.

Best Regards,
Hao Wu

> 
> CopyMem ((VOID *)&PreviousFile, (VOID *)File, sizeof (UDF_FILE_INFO));
> 
> Thanks,
> Star
> 
> >
> > Cc: Paulo Alcantara <paulo@paulo.ac>
> > Cc: Paulo Alcantara <palcantara@suse.de>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Hao Wu <hao.a.wu@intel.com>
> > ---
> >   .../Universal/Disk/UdfDxe/FileSystemOperations.c   | 30
> ++++++++++++++++++++--
> >   1 file changed, 28 insertions(+), 2 deletions(-)
> >
> > diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> > index b9ebddfe62..a89e5ba9ff 100644
> > --- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> > +++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> > @@ -2145,6 +2145,8 @@ ResolveSymlink (
> >     UINT8               CompressionId;
> >     UDF_FILE_INFO       PreviousFile;
> >
> > +  ZeroMem ((VOID *)File, sizeof (UDF_FILE_INFO));
> > +
> >     //
> >     // Symlink files on UDF volumes do not contain so much data other than
> >     // Path Components which resolves to real filenames, so it's OK to read
> in
> > @@ -2257,6 +2259,13 @@ ResolveSymlink (
> >         }
> >         FileName[Index] = L'\0';
> >         break;
> > +    default:
> > +      //
> > +      // Accoring to the ECMA-167 standard (3rd Edition - June 1997),
> Section
> > +      // 14.16.1.1, all other values are reserved.
> > +      //
> > +      Status = EFI_VOLUME_CORRUPTED;
> > +      goto Error_Find_File;
> >       }
> >
> >       //
> > @@ -2281,8 +2290,18 @@ ResolveSymlink (
> >         break;
> >       }
> >
> > -    if (CompareMem ((VOID *)&PreviousFile, (VOID *)Parent,
> > -                    sizeof (UDF_FILE_INFO)) != 0) {
> > +    //
> > +    // Check the content in the file info pointed by File.
> > +    //
> > +    if ((File->FileEntry == NULL) || (File->FileIdentifierDesc == NULL)) {
> > +      Status = EFI_VOLUME_CORRUPTED;
> > +      goto Error_Find_File;
> > +    }
> > +
> > +    if ((CompareMem ((VOID *)&PreviousFile, (VOID *)Parent,
> > +                    sizeof (UDF_FILE_INFO)) != 0) &&
> > +        (CompareMem ((VOID *)&PreviousFile, (VOID *)File,
> > +                    sizeof (UDF_FILE_INFO)) != 0)) {
> >         CleanupFileInformation (&PreviousFile);
> >       }
> >
> > @@ -2294,6 +2313,13 @@ ResolveSymlink (
> >     //
> >     FreePool (ReadFileInfo.FileData);
> >
> > +  //
> > +  // Check the content in the resolved file info.
> > +  //
> > +  if ((File->FileEntry == NULL) || (File->FileIdentifierDesc == NULL)) {
> > +    return EFI_VOLUME_CORRUPTED;
> > +  }
> > +
> >     return EFI_SUCCESS;
> >
> >   Error_Find_File:
> >