From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 9C9BE20945C17 for ; Fri, 8 Sep 2017 01:33:02 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id F3D5281DE4; Fri, 8 Sep 2017 08:35:53 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com F3D5281DE4 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-10.rdu2.redhat.com [10.10.120.10]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5E8FE614C0; Fri, 8 Sep 2017 08:35:51 +0000 (UTC) To: Paulo Alcantara , 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: From: Laszlo Ersek Message-ID: <8ca69ac9-4f5a-3aa9-f150-844b0aeeb898@redhat.com> Date: Fri, 8 Sep 2017 10:35:50 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Fri, 08 Sep 2017 08:35:54 +0000 (UTC) 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 08:33:02 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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