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 9B096203B99A8 for ; Mon, 9 Jul 2018 17:02:46 -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 2679B81A4EBF; Tue, 10 Jul 2018 00:02:45 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-21.rdu2.redhat.com [10.10.121.21]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0790C2026D65; Tue, 10 Jul 2018 00:02:43 +0000 (UTC) To: Roman Bacik , edk2-devel@lists.01.org Cc: Chao Zhang , Jiewen Yao , Vladimir Olovyannikov References: From: Laszlo Ersek Message-ID: <4e3b21af-4aeb-1524-df25-a1d7e08fdc94@redhat.com> Date: Tue, 10 Jul 2018 02:02:43 +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.8]); Tue, 10 Jul 2018 00:02:45 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Tue, 10 Jul 2018 00:02:45 +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 v1] SecurityPkg: Fix assert when setting key from FAT formatted 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: Tue, 10 Jul 2018 00:02:46 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit I'm not officially a reviewer for SecurityPkg, so just some light comments: (1) Can you send out the patch with git-send-email? Currently it looks like the patch was pasted into a desktop or web mail client, and that makes it hard to apply the patch (it's wrapped etc). On 07/10/18 00:11, Roman Bacik wrote: > 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 uint16 > aligned > boundary: https://bugzilla.tianocore.org/show_bug.cgi?id=1003 > > Cc: Chao Zhang > Cc: Jiewen Yao > Cc: Laszlo Ersek > Cc: Vladimir Olovyannikov > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Roman Bacik > --- > .../SecureBootConfigFileExplorer.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git > a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c > b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c > index 1b6f88804275..d5338406957c 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; > + UINT16 PathLength; > > if ((FilePath == NULL || FileHandle == NULL)) { > return EFI_INVALID_PARAMETER; > @@ -173,6 +175,10 @@ OpenFileByDevicePath( > // > Handle2 = Handle1; > Handle1 = NULL; > + PathLength = ((FILEPATH_DEVICE_PATH*)*FilePath)->Header.Length[0] | > + ((FILEPATH_DEVICE_PATH*)*FilePath)->Header.Length[1] << 8; (2) Can we use DevicePathNodeLength() from "MdePkg/Include/Library/DevicePathLib.h" here? (For that, we should also switch PathLength to UINTN.) This module already depends on the DevicePathLib class. Apologies that I didn't suggest this in the BZ. > + PathName = AllocateZeroPool (PathLength); > + CopyMem (PathName, ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName, > PathLength); (3) I think it's not necessary to zero-fill the buffer, we're going to overwrite it right after the allocation. There's a convenience function for that: AllocateCopyPool(). (4) The number of bytes is not correct IMO. "PathLength" stands for the number of bytes in the entire device path node (FILEPATH_DEVICE_PATH), including Header and PathName. So, for getting the number of bytes in just PathName, we should subtract the size of Header. Presently, we over-read the source buffer; it's not causing problems because PathName is NUL-terminated. (5) Can you please check whether the allocation succeeds? If it fails (PathName == NULL), we should return EFI_OUT_OF_RESOURCES. It seems OK to return this error from the loop body. > > // > // Try to test opening an existing file > @@ -180,7 +186,7 @@ OpenFileByDevicePath( > Status = Handle2->Open ( > Handle2, > &Handle1, > - ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName, > + PathName, > OpenMode &~EFI_FILE_MODE_CREATE, > 0 > ); > @@ -192,7 +198,7 @@ OpenFileByDevicePath( > Status = Handle2->Open ( > Handle2, > &Handle1, > - ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName, > + PathName, > OpenMode, > Attributes > ); > @@ -202,6 +208,8 @@ OpenFileByDevicePath( > // > Handle2->Close (Handle2); > > + FreePool (PathName); > + > if (EFI_ERROR(Status)) { > return (Status); > } > Right, this logic appears fine to me; no leaks. Thanks! Laszlo