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

[Laszlo Ersek <lersek@redhat.com>]

Paulo,

On 09/08/17 02:56, Paulo Alcantara wrote:
> Ray,
>
> On 07/09/2017 20:13, Paulo Alcantara wrote:
>> v5:
>>   - Fixed OVMF IA32 build.
>>   - Fixed a typo in UdfDriveBindingStop() ("This" -> "SimpleFs") which
>>     broke retrieval of private fs data from SimpleFs protocol --
>>     identified by 'reconnect -r' command in UEFI shell.
>
> Follow the diff between v4 and v5 for Mde*Pkg changes (forgot to include
> it when resending):
>
> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> index 8ad14fe594..2dbcff0be4 100644
> --- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> @@ -372,7 +372,7 @@ UdfRead (
>        PrivFileData->FileSize,
>        &PrivFileData->FilePosition,
>        Buffer,
> -      BufferSize
> +      (UINT64 *)(UINTN)BufferSize^M
>        );

This change is not correct.

(1) The UdfRead() function takes the following parameter:

  IN OUT  UINTN              *BufferSize,

This means that, in an IA32 or ARM build,

  sizeof *BufferSize == 4

and in an AARCH64 or X64 build,

  sizeof *BufferSize == 8

(2) The above type-casting is part of a call to the ReadFileData()
function. The ReadFileData() function takes the following parameter:

  IN OUT  UINT64                 *BufferSize

This means that, regardless of architecture,

  sizeof *BufferSize == 8

The consequence is that, in an IA32 or ARM build, the ReadFileData()
function will both read and write beyond the end of the outermost
caller's "BufferSize" variable. The write is a problem without a doubt,
but the read is a problem too if the outermost caller's "BufferSize" (a
UINT32 object) is followed by four bytes that are not all zero. Then
ReadFileData() will attempt to read more than 4GB of data.

The right way to fix this is the following:

> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> index 2dbcff0be4a3..07c7ec207fcd 100644
> --- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> @@ -326,6 +326,7 @@ UdfRead (
>    VOID                            *NewFileEntryData;
>    CHAR16                          FileName[UDF_FILENAME_LENGTH] = { 0 };
>    UINT64                          FileSize;
> +  UINT64                          BufferSizeUint64;
>  
>    OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
>  
> @@ -364,6 +365,7 @@ UdfRead (
>        goto Done;
>      }
>  
> +    BufferSizeUint64 = *BufferSize;
>      Status = ReadFileData (
>        BlockIo,
>        DiskIo,
> @@ -372,8 +374,10 @@ UdfRead (
>        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;

Thanks,
Laszlo

>    } else if (IS_FID_DIRECTORY_FILE (Parent->FileIdentifierDesc)) {
>      if (ReadDirInfo->FidOffset == 0 && PrivFileData->FilePosition > 0) {
> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/Udf.c
> b/MdeModulePkg/Universal/Disk/UdfDxe/Udf.c
> index 9f10c78ca9..49dc7077b7 100644
> --- a/MdeModulePkg/Universal/Disk/UdfDxe/Udf.c
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/Udf.c
> @@ -264,7 +264,7 @@ UdfDriverBindingStop (
>      EFI_OPEN_PROTOCOL_GET_PROTOCOL
>      );
>    if (!EFI_ERROR (Status)) {
> -    PrivFsData = PRIVATE_UDF_SIMPLE_FS_DATA_FROM_THIS (This);
> +    PrivFsData = PRIVATE_UDF_SIMPLE_FS_DATA_FROM_THIS (SimpleFs);^M
>
>      //
>      // Uninstall child handle
>
>
> Thanks,
> Paulo