From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.120; helo=mga04.intel.com; envelope-from=jiewen.yao@intel.com; receiver=edk2-devel@lists.01.org Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) (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 CB338207DF287 for ; Thu, 12 Jul 2018 05:08:11 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 12 Jul 2018 05:08:11 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,342,1526367600"; d="scan'208";a="53842539" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga007.fm.intel.com with ESMTP; 12 Jul 2018 05:07:54 -0700 Received: from fmsmsx158.amr.corp.intel.com (10.18.116.75) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 12 Jul 2018 05:07:47 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx158.amr.corp.intel.com (10.18.116.75) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 12 Jul 2018 05:07:47 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.124]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.173]) with mapi id 14.03.0319.002; Thu, 12 Jul 2018 20:07:45 +0800 From: "Yao, Jiewen" To: Laszlo Ersek CC: Roman Bacik , "Zhang, Chao B" , Vladimir Olovyannikov , "Justen, Jordan L" , "edk2-devel@lists.01.org" , "Gao, Liming" , "Kinney, Michael D" , "Yao, Jiewen" Thread-Topic: [edk2] [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB Thread-Index: AQHUGKCBhiamTwZQ50m2Jv2sVGai2KSJZ50AgAA9KoCAAAYIAIAAU88AgAGCA48= Date: Thu, 12 Jul 2018 12:07:45 +0000 Message-ID: <995927A3-8CBC-4E92-B1B5-5DCEA451328D@intel.com> References: <20180710225105.28443-1-roman.bacik@broadcom.com> <5bca3f43-7c23-dca6-03cd-2d647d8fe253@redhat.com> , <415f1063-7a67-b5ec-cc84-83e1b36cb46a@redhat.com> In-Reply-To: <415f1063-7a67-b5ec-cc84-83e1b36cb46a@redhat.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: MIME-Version: 1.0 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: Thu, 12 Jul 2018 12:08:12 -0000 Content-Language: zh-CN Content-Type: text/plain; charset="iso-2022-jp" Content-Transfer-Encoding: quoted-printable thanks laszlo=20 I like your idea to eliminate the code duplication and fix all the problem = by adding a new api in uefi lib.=20 If there is urgency to fix this specific issue, I have no problem on the en= hancement.=20 Reviewed by : Jiewen.yao@intel.com thank you! Yao, Jiewen > =1B$B:_=1B(B 2018=1B$BG/=1B(B7=1B$B7n=1B(B12=1B$BF|!$>e8a=1B(B5:06=1B$B!$= =1B(BLaszlo Ersek =1B$B=20 >> On 07/11/18 18:06, Laszlo Ersek wrote: >>> On 07/11/18 17:44, Roman Bacik wrote: >>> Hi Laszlo, >>>=20 >>> Thank you very much for your review and help. I would prefer the option= 2b. >>=20 >> Great, thanks! Let's wait for the SecurityPkg maintainers then, to give >> their R-b's for your patch. Chao Zhang, Jiewen, can you please comment? >>=20 >> From my side, dependent on the pending commit message and patch >> whitespace corrections (which I'm willing to implement myself, at push): >>=20 >> Reviewed-by: Laszlo Ersek >=20 > Here's the TianoCore BZ that I plan to reference in the updated commit > message, as "further known problems": >=20 > https://bugzilla.tianocore.org/show_bug.cgi?id=3D1008 >=20 > Thanks > Laszlo >=20 >=20 >>> Thanks, >>>=20 >>> Roman >>>=20 >>>> On Wed, Jul 11, 2018 at 5:05 AM, Laszlo Ersek wrot= e: >>>>=20 >>>> Hi Roman, >>>>=20 >>>>> On 07/11/18 00:51, rbacik@gmail.com wrote: >>>>> From: Roman Bacik >>>>>=20 >>>>> When secure boot is enabled, if one loads keys from a FAT formatted >>>>> eMMC/SD/USB when trying to provision PK/KEK/DB keys via the menu, >>>>> an assert in StrLen() occurs. >>>>> This is because the filename starts on odd address, which is not a ui= nt16 >>>>> aligned boundary: https://bugzilla.tianocore.org/show_bug.cgi?id=3D10= 03 >>>>>=20 >>>>> Cc: Chao Zhang >>>>> Cc: Jiewen Yao >>>>> Cc: Laszlo Ersek >>>>> Cc: Vladimir Olovyannikov >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>> Signed-off-by: Roman Bacik >>>>> --- >>>>> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfi= gFileExplorer.c >>>> | 13 +++++++++++-- >>>>> 1 file changed, 11 insertions(+), 2 deletions(-) >>>>=20 >>>> Thank you for sending a well-formed patch. >>>>=20 >>>> I notice that you sent this email from , which is no= t >>>> the same as the Signed-off-by line. I realize you posted from >>>> for technical reasons, and it should be no problem. >>>>=20 >>>> However, I *think* in such cases we usually request the following: >>>>=20 >>>> - Using your broadcom.com email address, please respond to this patch >>>> (not my present email, but your original git posting), keeping full >>>> context, and just repeat your Signed-off-by line (referencing the >>>> broadcom address). >>>>=20 >>>> I'm CC'ing Jordan and Ard for confirmation -- I believe this is what >>>> we've done in the past, in cases when submitters had to post their wor= k >>>> from private addresses due to company email issues. >>>>=20 >>>> Technical comments below: >>>>=20 >>>>> diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/Se= cureBootConfigFileExplorer.c >>>> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/ >>>> SecureBootConfigFileExplorer.c >>>>> index 1b6f88804275..19b13a5569a6 100644 >>>>> --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/ >>>> SecureBootConfigFileExplorer.c >>>>> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/ >>>> SecureBootConfigFileExplorer.c >>>>> @@ -123,6 +123,8 @@ OpenFileByDevicePath( >>>>> EFI_FILE_PROTOCOL *Handle1; >>>>> EFI_FILE_PROTOCOL *Handle2; >>>>> EFI_HANDLE DeviceHandle; >>>>> + CHAR16 *PathName; >>>>> + UINTN PathLength; >>>>>=20 >>>>> if ((FilePath =3D=3D NULL || FileHandle =3D=3D NULL)) { >>>>> return EFI_INVALID_PARAMETER; >>>>> @@ -173,6 +175,11 @@ OpenFileByDevicePath( >>>>> // >>>>> Handle2 =3D Handle1; >>>>> Handle1 =3D NULL; >>>>> + PathLength =3D DevicePathNodeLength(*FilePath) - >>>> sizeof(EFI_DEVICE_PATH_PROTOCOL); >>>>> + PathName =3D AllocateCopyPool(PathLength, ((FILEPATH_DEVICE_PATH= *)* >>>> FilePath)->PathName); >>>>=20 >>>> (1) On both lines above, space characters are missing after: >>>> DevicePathNodeLength, sizeof, and AllocateCopyPool. (Edk2 coding style= .) >>>> I think we can fix this up for you when we push the patch. (I'm willin= g >>>> to help with that, but we need SecurityPkg maintainer review first.) >>>>=20 >>>>=20 >>>>> + if (PathName =3D=3D NULL) { >>>>> + return EFI_OUT_OF_RESOURCES; >>>>> + } >>>>=20 >>>> (2) I have now reviewed the original state of the function more >>>> carefully, and, while the above "return" branch introduces a leak >>>> *path*, it does not introduce a leak that doesn't already exist! >>>>=20 >>>> In fact, the original function has multiple issues: >>>>=20 >>>> - If the OpenVolume() call fails, "FileHandle" is set to NULL. That's >>>> useless; the intent is obviously to set (*FileHandle) to NULL. >>>>=20 >>>> - At the top of the "while" loop body, "Handle1" stands for an open >>>> EFI_FILE_PROTOCOL. If the device path type check at the top of the loo= p >>>> body returns EFI_INVALID_PARAMETER, then it (a) performs the same >>>> useless assignment to "FileHandle" as described above, and (b) fails t= o >>>> close "Handle1". This is why I say that the above EFI_OUT_OF_RESOURCES >>>> branch introduces no new leak, just a new path to the existent leak. >>>>=20 >>>> - The OpenFileByDevicePath() function is duplicated in the following >>>> modules: "NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c", and >>>> "MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c". With t= he >>>> implication that the alignment issue you found affects all three drive= rs! >>>>=20 >>>>=20 >>>> Roman, I realize this could be more than what you signed up for; so >>>> please pick one: >>>>=20 >>>> (2a) you could submit a patch series: >>>>=20 >>>> * Write a patch that sets (*FilePath) to NULL right after the >>>> (FileHandle=3D=3DNULL) check, in preparation for failure, and removes = all >>>> the bogus FileHandle=3DNULL assignments. >>>>=20 >>>> * Write another patch that plugs the leak when the device path type >>>> check fails -- introduce a "CloseHandle1" label at the end of the >>>> function, and jump to it when the devpath type check fails, so that we >>>> close "Handle1". This patch should also invert the meanings of Handle2 >>>> and Handle1 -- the reassignment to Handle1 should only occur *after* w= e >>>> successfully open Handle2. "Handle1" should *always* remain suitable f= or >>>> closing through the "CloseHandle1" error path. >>>>=20 >>>> * Include your current patch, for fixing the alignment issue. >>>>=20 >>>> * Write another patch that moves the OpenFileByDevicePath() function t= o >>>> UefiLib in MdePkg -- under the name EfiOpenFileByDevicePath() -- from >>>> SecureBootConfigDxe. >>>>=20 >>>> * write two more patches, namely for TlsAuthConfigDxe and RamDiskDxe, = in >>>> order to consume EfiOpenFileByDevicePath() from UefiLib. Both of those >>>> modules already depend on UefiLib. >>>>=20 >>>> (2b) Alternatively: >>>>=20 >>>> * we can report a new TianoCore BZ about the issues I list above, >>>>=20 >>>> * we can commit this patch of yours as-is, just additionally reference >>>> the *new* BZ in the commit message, as "further known issues", >>>>=20 >>>> * I can work on the rest of the issues. >>>>=20 >>>>=20 >>>> If you pick (2b), then I can >>>> - file the new BZ, >>>> - update the commit message for you, >>>> - update the patch for you, as described in (1), >>>> - ACK this patch (as updated above), >>>> - push the patch (if SecurityPkg maintainers agree), >>>> - take on the new BZ as well. >>>>=20 >>>> Thanks! >>>> Laszlo >>>>=20 >>>>>=20 >>>>> // >>>>> // Try to test opening an existing file >>>>> @@ -180,7 +187,7 @@ OpenFileByDevicePath( >>>>> Status =3D Handle2->Open ( >>>>> Handle2, >>>>> &Handle1, >>>>> - ((FILEPATH_DEVICE_PATH*)*FilePath)->PathNa= me, >>>>> + PathName, >>>>> OpenMode &~EFI_FILE_MODE_CREATE, >>>>> 0 >>>>> ); >>>>> @@ -192,7 +199,7 @@ OpenFileByDevicePath( >>>>> Status =3D Handle2->Open ( >>>>> Handle2, >>>>> &Handle1, >>>>> - ((FILEPATH_DEVICE_PATH*)* >>>> FilePath)->PathName, >>>>> + PathName, >>>>> OpenMode, >>>>> Attributes >>>>> ); >>>>> @@ -202,6 +209,8 @@ OpenFileByDevicePath( >>>>> // >>>>> Handle2->Close (Handle2); >>>>>=20 >>>>> + FreePool (PathName); >>>>> + >>>>> if (EFI_ERROR(Status)) { >>>>> return (Status); >>>>> } >>>>>=20 >>>>=20 >>>> _______________________________________________ >>>> edk2-devel mailing list >>>> edk2-devel@lists.01.org >>>> https://lists.01.org/mailman/listinfo/edk2-devel >>>>=20 >>>=20 >>=20 >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel >>=20 >=20