From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (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 0715B20945B65 for ; Sat, 16 Sep 2017 16:49:48 -0700 (PDT) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga105.jf.intel.com with ESMTP; 16 Sep 2017 16:52:49 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,405,1500966000"; d="scan'208,217";a="1015198212" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by orsmga003.jf.intel.com with ESMTP; 16 Sep 2017 16:52:49 -0700 Received: from fmsmsx152.amr.corp.intel.com (10.18.125.5) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sat, 16 Sep 2017 16:52:48 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX152.amr.corp.intel.com (10.18.125.5) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sat, 16 Sep 2017 16:52:47 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.175]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.159]) with mapi id 14.03.0319.002; Sun, 17 Sep 2017 07:52:45 +0800 From: "Yao, Jiewen" To: Laszlo Ersek , Paulo Alcantara CC: "Ni, Ruiyu" , "Dong, Eric" , "edk2-devel@lists.01.org" , "Gao, Liming" , "Kinney, Michael D" , "Zeng, Star" Thread-Topic: [edk2] [PATCH 0/3] UDF partition driver fix Thread-Index: AQHTLzRrxmS909xN8k6aPBu8ozZP6KK3jn4AgACSPEA= Date: Sat, 16 Sep 2017 23:52:45 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503A9BC765@shsmsx102.ccr.corp.intel.com> References: <4d74e669-964e-7910-2de6-b7e831f9c2eb@redhat.com> In-Reply-To: <4d74e669-964e-7910-2de6-b7e831f9c2eb@redhat.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 X-Content-Filtered-By: Mailman/MimeDel 2.1.22 Subject: Re: [PATCH 0/3] UDF partition driver fix 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: Sat, 16 Sep 2017 23:49:48 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Thank you Paulo, to provide a fix for this driver. I do not have comment for this specific patch. I would defer the review wor= k to Star and Ruiyu. I do have some general question for the new UDF support and I would like to= know more detail about the quality level. As we are seeing some issues in the new UDF driver, would you please share = what test you have done for the UDF support? (Not only for this patch, but = also for the UDF partition and UDF file system which are already checked in= ) I ask this specially, because UDF support (partition and file system) adds = a brand new group of external input for the UEFI BIOS. For a long time, we = are monitoring all the external input. Per our security model, the external input means the input by an end user. = The known external includes but not limited to UEFI image (OROM/OS Loader),= Capsule Image, Recovery Image, file system, partition, network packet, var= iable, etc. UDF is a new one, because with the new UDF support, now a malicious user ma= y insert a mal-format UDF CDROM to the system and try to break the system. = As such, we need evaluate it. To be specific, would you please share: 1) Which UDF spec these 2 drivers (FS and partition) are following? I have seen you mentioned the header file follows "(revisions 1.02 through = 2.60)". But I am not sure about the driver. I found you mentioned: Originally the driver was written to support UDF file systems as specified by OSTA Universal Disk Format Specification 2.60. However, some Windows 10 Enterprise ISO (UDF bridge) images that I tested supported a revision of 1.02 thus I had to rework the driver a little bit to support such revision as well. Do you mean you only support 1.02 and 2.6 in driver, or you support 1.02 th= rough 2.6? 2) Which UDF function is supported? And more important, which UDF fun= ction is NOT supported? I have seen "Compliance" section in UDF spec, and it lists some optional fe= ature, such as multi-volume, multi-partition, multisession, file name trans= lation, backward read, backward write, etc. I also have interest to know the support level of current existing CDROM, a= nd existing UDF driver in OS (such as Windows, or Linux). How many optional= feature are implemented? I ask this, because we want to understand how we declare the support level = of this UEFI UDF driver. If we just say we support UDF, then naive people m= ay believe we support everything. :) 3) Which compatibility test has been done? I am sorry that I cannot find the first version patch. I fund you mentioned= Win10 ISO is tried in V2. Any more? We would like to know how many existing OS installation CDROM (or any other= CDROM) has been tried. Such as Linux (RedHat, Ubuntu, Suse, etc), or Windo= ws (Win7, Win8, Win10)? The more detail, the better. May a list. 4) The last but not least important, which negative test (security te= st) has been done? Have you run some fuzzing test? Have you tried a mal-format UDF disc? For example, with bad offset or lengt= h? Have you test the integer overflow / buffer over flow handing in the code? NOTE: we should not use ASSERT to handle such error. When I review the code, I found below: Status =3D ReadFileData ( BlockIo, DiskIo, Volume, Parent, PrivFileData->FileSize, &PrivFileData->FilePosition, Buffer, &BufferSizeUint64 ); ASSERT (BufferSizeUint64 <=3D MAX_UINTN); *BufferSize =3D (UINTN)BufferSizeUint64; I am not sure if we can use ASSERT (BufferSizeUint64 <=3D MAX_UINTN); Can a malicious user construct a bad UDF and make BufferSizeUint64 > MAX_UI= NTN? Does it might happen? Or never happen? We documented Appendix B - EDKII code review top 5 in https://github.com/ti= anocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Security_Desig= n_Guide_in_EDK_II.pdf 3 of them are matched in these partition and file system drivers. I quote b= elow: =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D If the code consumes input from an untrusted source or region, Ensure that the input is rigorously and adequately validated. Verify buffer overflow is handled. Avoid integer overflow. Try to use subtraction instead of addition and division instead of multipli= cation. Verify that ASSERT is used properly. ASSERT is used to catch conditions that should never happen. If something m= ight happen, use error handling instead. We can use both ASSERT and error h= andling to facilitate debugging - ASSERT allows for earlier detection and i= solation of several classes of issues. [McConnell] =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D It is just a reminder. If you are already following that, it will be great.= Please let us now. I take a glance of UDF 2.6 specification, but I do not have chance to read = all content at this moment. If I asked some stupid question , please feel free to correct me. All in all, we hope to understand the current quality level of the UDF part= ition support and UDF file system. If you can help to provide the detail, it may help us to evaluate. Thank you Yao Jiewen From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Lasz= lo Ersek Sent: Sunday, September 17, 2017 6:17 AM To: Paulo Alcantara Cc: Ni, Ruiyu ; Dong, Eric ; edk2-= devel@lists.01.org; Gao, Liming ; Kinney, Michael D <= michael.d.kinney@intel.com>; Zeng, Star Subject: Re: [edk2] [PATCH 0/3] UDF partition driver fix Hi Paulo, On 09/16/17 23:36, Paulo Alcantara wrote: > This series fixes an UDF issue with Partition driver as discussed in ML: > > https://lists.01.org/pipermail/edk2-devel/2017-September/014694.html > > Thanks! > Paulo > > Repo: https://github.com/pcacjr/edk2.git > Branch: udf-partition-fix > > Paulo Alcantara (3): > MdePkg: Add UDF volume structure definitions > MdeModulePkg/PartitionDxe: Fix creation of UDF logical partition > MdeModulePkg/UdfDxe: Rework driver to support PartitionDxe changes > > MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 307 +++++++++++- > MdeModulePkg/Universal/Disk/UdfDxe/File.c | 13 +- > .../Universal/Disk/UdfDxe/FileSystemOperations.c | 525 ++++++++-------= ------ > MdeModulePkg/Universal/Disk/UdfDxe/Udf.c | 7 - > MdeModulePkg/Universal/Disk/UdfDxe/Udf.h | 88 +--- > MdePkg/Include/IndustryStandard/Udf.h | 63 +++ > 6 files changed, 560 insertions(+), 443 deletions(-) > Thank you very much for submitting this patchset quickly. I hope it will work out, and we won't need the PartitionExperimentalDxe.inf file! I have some trivial process-level suggestions: * when submitting a patchset, please collect the Cc: tags from all the commit messages, and add them to the cover letter manually. This way everybody you CC on at least some of the patches will get the cover letter too, presonally. This matters because otherwise replies to the blurb will also miss those people, personally. (I'm now adding everyone manually.) * Because edk2 uses long directory and file names, the diffstats are frequently truncated like above (see "..."). You can avoid this if you format the patches like this: --stat=3D1000 --stat-graph-width=3D20 this will make the pathname column just as wide as necessary, and will also keep the chart to the right reasonably narrow. * It's probably best to include a reference to in the commit messages (in particular patch #2). * Once you post a patchset for a TianoCore BZ, it's useful to link the series (from the mailing list archive) in the BZ itself. Regarding the code itself, I don't think I can help here in any sensible way. (If UDF support were located under OvmfPkg, I would totally consider you the owner of those files, verify your patches for them on a formal level only, and if that part was fine, I'd give an Acked-by.) Thanks! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel