From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 3627620968904 for ; Wed, 11 Jul 2018 10:25:01 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1557E9BA94; Wed, 11 Jul 2018 17:25:01 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-200.rdu2.redhat.com [10.10.121.200]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7B4F62026D6B; Wed, 11 Jul 2018 17:24:59 +0000 (UTC) To: "Carsey, Jaben" , "rbacik@gmail.com" , "edk2-devel@lists.01.org" Cc: "Ni, Ruiyu" , Vladimir Olovyannikov , "Justen, Jordan L" , "Gao, Liming" , "Yao, Jiewen" , "Kinney, Michael D" , "Zhang, Chao B" References: <20180710225105.28443-1-roman.bacik@broadcom.com> <5bca3f43-7c23-dca6-03cd-2d647d8fe253@redhat.com> From: Laszlo Ersek Message-ID: <22490199-78bc-d1ec-a12c-16332b877772@redhat.com> Date: Wed, 11 Jul 2018 19:24:58 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Wed, 11 Jul 2018 17:25:01 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Wed, 11 Jul 2018 17:25:01 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 11 Jul 2018 17:25:02 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 07/11/18 19:10, Carsey, Jaben wrote: > >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >> Laszlo Ersek >> Sent: Wednesday, July 11, 2018 5:16 AM >> To: rbacik@gmail.com; edk2-devel@lists.01.org >> Cc: Ni, Ruiyu ; Vladimir Olovyannikov >> ; Justen, Jordan L >> ; Gao, Liming ; Carsey, >> Jaben ; Yao, Jiewen ; >> Kinney, Michael D ; Zhang, Chao B >> >> Subject: Re: [edk2] [PATCH v2] SecurityPkg: Fix assert when setting key from >> eMMC/SD/USB >> Importance: High >> >> On 07/11/18 14:05, Laszlo Ersek wrote: >> >>> - The OpenFileByDevicePath() function is duplicated in the following >>> modules: "NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c", and >>> "MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c". >> With the >>> implication that the alignment issue you found affects all three drivers! >> >> Note that I've mutually diffed the three function definitions between >> each other (with "diff -b"), and there are only whitespace and comment >> wrapping differences. >> >> So, again, this function looks like a prime suspect for UefiLib. >> >> I do see the strange reference at the end ("undefined SHELL_FILE_HANDLE >> format"); I think that's simply stale, and should be removed. > > Looks like the stale reference may be from origination as derived from ShellOpenFileByDevicePath. I agree this should be moved to a lib. Any way we could merge the shell's only slightly different version also into the mix? Wow, that's incredible -- ShellOpenFileByDevicePath() contains the *exact same* alignment fix that Roman is contributing with this patch; from commit 0b6cb335fa82 ("Fixed some alignment faults in IPF platform", 2013-01-25). ShellOpenFileByDevicePath() -- including the alignment fix -- also has the same kind of resource leak (regarding "Handle1"). So, code duplication has bitten us again. We have four instances of basically the same logic in the tree, all four are affected by the same resource leak, and the alignment fix that was discovered in or before 2013 was applied to only one of the four. So, yes. The top of the ShellOpenFileByDevicePath() function (the "UEFI Shell 2.0 method") should be preserved, but then the last part ("use old shell method") should be replaced with a call to the new UefiLib API. And, the final "weak spot" comment remains valid, but only for ShellOpenFileByDevicePath(), not for the new EfiOpenFileByDevicePath() API. Thank you Jaben for finding this! Laszlo