From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.zytor.com (terminus.zytor.com [65.50.211.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B44A420945C19 for ; Fri, 8 Sep 2017 04:45:30 -0700 (PDT) Received: from [IPv6:2804:7f4:c480:9cd0::2] ([IPv6:2804:7f4:c480:9cd0:0:0:0:2]) (authenticated bits=0) by mail.zytor.com (8.15.2/8.15.2) with ESMTPSA id v88Bk3ov020894 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Fri, 8 Sep 2017 04:46:06 -0700 To: Laszlo Ersek , edk2-devel@lists.01.org Cc: Jordan Justen , Andrew Fish , Michael D Kinney , Liming Gao , Star Zeng , Eric Dong , Mark Doran , Ruiyu Ni , hao.a.wu@intel.com References: <8ca69ac9-4f5a-3aa9-f150-844b0aeeb898@redhat.com> From: Paulo Alcantara Message-ID: <17fde1d5-c9ec-2a42-87a0-136cdef90861@zytor.com> Date: Fri, 8 Sep 2017 08:46:01 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <8ca69ac9-4f5a-3aa9-f150-844b0aeeb898@redhat.com> Subject: Re: [PATCH v5 0/6] read-only UDF file system support X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 08 Sep 2017 11:45:30 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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