Re: [PATCH v6 0/6] read-only UDF file system support

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Friday, September 8, 2017 9:26 PM
To: Paulo Alcantara <pcacjr@zytor.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
Cc: Dong, Eric <eric.dong@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Andrew Fish <afish@apple.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] [PATCH v6 0/6] read-only UDF file system support

Ray,

On 09/08/17 14:41, Paulo Alcantara wrote:

> v6:
>  - Fixed a bug in UdfRead() that'd pontentially break in ARM or IA32
>    by allowing caller to read more than 4GiB of data
>    (i.e. BufferSize pointer is dereferenced as an UINT64 * and it's
>     followed by 4 bytes that are nonzero).
>
> Repo:   https://github.com/pcacjr/edk2.git
> Branch: udf-fs-v6

The v5-v6 diff is as follows:

> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c 
> b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> index 2dbcff0be4a3..8b9339567f8e 100644
> --- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> @@ -325,8 +325,9 @@ UdfRead (
>    UDF_FILE_IDENTIFIER_DESCRIPTOR  *NewFileIdentifierDesc;
>    VOID                            *NewFileEntryData;
>    CHAR16                          FileName[UDF_FILENAME_LENGTH] = { 0 };
>    UINT64                          FileSize;
> +  UINT64                          BufferSizeUint64;
>
>    OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
>
>    if (This == NULL || BufferSize == NULL || (*BufferSize != 0 && @@ 
> -363,18 +364,22 @@ UdfRead (
>        Status = EFI_SUCCESS;
>        goto Done;
>      }
>
> +    BufferSizeUint64 = *BufferSize;
> +
>      Status = ReadFileData (
>        BlockIo,
>        DiskIo,
>        Volume,
>        Parent,
>        PrivFileData->FileSize,
>        &PrivFileData->FilePosition,
>        Buffer,
> -      (UINT64 *)(UINTN)BufferSize
> +      &BufferSizeUint64
>        );
> +    ASSERT (BufferSizeUint64 <= MAX_UINTN);
> +    *BufferSize = (UINTN)BufferSizeUint64;
>    } else if (IS_FID_DIRECTORY_FILE (Parent->FileIdentifierDesc)) {
>      if (ReadDirInfo->FidOffset == 0 && PrivFileData->FilePosition > 0) {
>        Status = EFI_DEVICE_ERROR;
>        *BufferSize = 0;

It looks OK to me, and it builds fine for IA32, X64, ARM and AARCH64:

  Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/UdfDxe.efi
  Build/ArmVirtQemu-ARM/DEBUG_GCC5/ARM/UdfDxe.efi
  Build/OvmfIa32/NOOPT_GCC48/IA32/UdfDxe.efi
  Build/OvmfX64/NOOPT_GCC48/X64/UdfDxe.efi

Green light from your side?

Paulo: you forgot to pick up Ray's R-b for patches #4 and #5, from his
v5 response
<http://mid.mail-archive.com/734D49CCEBEEF84792F5B80ED585239D5BA282B7@SHSMSX104.ccr.corp.intel.com>
-- it was for the entire series.

But, I'll apply that for you.

Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel