* Re: [PATCH] MdeModulePkg/UdfDxe: Additional checks for ResolveSymlink()
2018-10-26 7:54 [PATCH] MdeModulePkg/UdfDxe: Additional checks for ResolveSymlink() Hao Wu
@ 2018-10-26 13:26 ` Paulo Alcantara
2018-10-26 14:14 ` Zeng, Star
2018-10-29 11:09 ` Leif Lindholm
2 siblings, 0 replies; 6+ messages in thread
From: Paulo Alcantara @ 2018-10-26 13:26 UTC (permalink / raw)
To: Hao Wu, edk2-devel; +Cc: Hao Wu, Ruiyu Ni, Star Zeng
Hao Wu <hao.a.wu@intel.com> writes:
> 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.
>
> 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:
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Thanks!
Paulo
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] MdeModulePkg/UdfDxe: Additional checks for ResolveSymlink()
2018-10-26 7:54 [PATCH] MdeModulePkg/UdfDxe: Additional checks for ResolveSymlink() Hao Wu
2018-10-26 13:26 ` Paulo Alcantara
@ 2018-10-26 14:14 ` Zeng, Star
2018-10-27 12:28 ` Wu, Hao A
2018-10-29 11:09 ` Leif Lindholm
2 siblings, 1 reply; 6+ messages in thread
From: Zeng, Star @ 2018-10-26 14:14 UTC (permalink / raw)
To: Hao Wu, edk2-devel; +Cc: Ruiyu Ni, Paulo Alcantara, star.zeng
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?
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:
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] MdeModulePkg/UdfDxe: Additional checks for ResolveSymlink()
2018-10-26 14:14 ` Zeng, Star
@ 2018-10-27 12:28 ` Wu, Hao A
0 siblings, 0 replies; 6+ messages in thread
From: Wu, Hao A @ 2018-10-27 12:28 UTC (permalink / raw)
To: Zeng, Star, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Paulo Alcantara
> -----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:
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] MdeModulePkg/UdfDxe: Additional checks for ResolveSymlink()
2018-10-26 7:54 [PATCH] MdeModulePkg/UdfDxe: Additional checks for ResolveSymlink() Hao Wu
2018-10-26 13:26 ` Paulo Alcantara
2018-10-26 14:14 ` Zeng, Star
@ 2018-10-29 11:09 ` Leif Lindholm
2018-10-30 1:01 ` Wu, Hao A
2 siblings, 1 reply; 6+ messages in thread
From: Leif Lindholm @ 2018-10-29 11:09 UTC (permalink / raw)
To: Hao Wu; +Cc: edk2-devel, Ruiyu Ni, Star Zeng, Paulo Alcantara
Hi Hao Wu,
I have no comment about the code changes, but would it be possible to
split this up into three separate patches? They are three separate
changes, but as it stands, it is not _obvious_ that they do not have
interdependencies. Splitting them up would make that immediately
obvious. You already have the three commit messages :)
Best Regards,
Leif
On Fri, Oct 26, 2018 at 03:54:57PM +0800, 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.
>
> 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:
> --
> 2.12.0.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] MdeModulePkg/UdfDxe: Additional checks for ResolveSymlink()
2018-10-29 11:09 ` Leif Lindholm
@ 2018-10-30 1:01 ` Wu, Hao A
0 siblings, 0 replies; 6+ messages in thread
From: Wu, Hao A @ 2018-10-30 1:01 UTC (permalink / raw)
To: Leif Lindholm
Cc: Ni, Ruiyu, Paulo Alcantara, edk2-devel@lists.01.org, Zeng, Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Leif Lindholm
> Sent: Monday, October 29, 2018 7:10 PM
> To: Wu, Hao A
> Cc: Ni, Ruiyu; Paulo Alcantara; edk2-devel@lists.01.org; Zeng, Star
> Subject: Re: [edk2] [PATCH] MdeModulePkg/UdfDxe: Additional checks for
> ResolveSymlink()
>
> Hi Hao Wu,
>
> I have no comment about the code changes, but would it be possible to
> split this up into three separate patches? They are three separate
> changes, but as it stands, it is not _obvious_ that they do not have
> interdependencies. Splitting them up would make that immediately
> obvious. You already have the three commit messages :)
Thanks Leif,
Yes, I will send a new series to address it.
Best Regards,
Hao Wu
>
> Best Regards,
>
> Leif
>
> On Fri, Oct 26, 2018 at 03:54:57PM +0800, 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.
> >
> > 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:
> > --
> > 2.12.0.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 6+ messages in thread