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

[Paulo Alcantara <pcacjr@zytor.com>]

On 08/09/2017 05:35, Laszlo Ersek wrote:
> 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.

You're right. Thanks for the explanation.

> 
> 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;

I'll include this in v6.

Paulo