From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org 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 9E61B21194865 for ; Thu, 22 Nov 2018 10:35:41 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 23506307C947; Thu, 22 Nov 2018 18:35:41 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-200.rdu2.redhat.com [10.10.120.200]) by smtp.corp.redhat.com (Postfix) with ESMTP id 530145D772; Thu, 22 Nov 2018 18:35:36 +0000 (UTC) To: Ard Biesheuvel , edk2-devel@lists.01.org Cc: leif.lindholm@linaro.org, thomas.abraham@arm.com, nariman.poushin@linaro.org, philmd@redhat.com References: <20181122172645.20819-1-ard.biesheuvel@linaro.org> <20181122172645.20819-5-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: Date: Thu, 22 Nov 2018 19:35:35 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181122172645.20819-5-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.46]); Thu, 22 Nov 2018 18:35:41 +0000 (UTC) Subject: Re: [PATCH edk2-platforms 4/4] Platform/ARM/BdsLib: maintain alignment for DevicePaths X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Nov 2018 18:35:41 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/22/18 18:26, Ard Biesheuvel wrote: > DevicePath node types may have any size, and so it is up to the > code that manipulates them to ensure that dereferencing them only > occurs when the pointer is aligned explicitly. > > Since BdsConnectAndUpdateDevicePath() has only two callers, at d9e68a756cfb ("Platform/ARM/SgiPkg: increase max variable size to 8KB", 2018-11-20), it seems to have three callers: - itself - BdsConnectDevicePath() - BdsLoadImageAndUpdateDevicePath() > one of > which itself, we can simply duplicate the device path (similar to > how DxeCore's CoreConnectController () does it), and free the pool > allocation again on the way out. (Note that the allocation only > occurs when the non-recursive path is taken) I think this rather works around than fixes the problem -- just because every remaining device path "slice" is realigned as we advance, it's not guaranteed that any and all CHAR16 fields in the now-first node will be naturally aligned. ... However, it certainly applies to FILEPATH_DEVICE_PATH.PathName, which is likely the only such field that we care about. :) > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel > --- > Platform/ARM/Library/BdsLib/BdsFilePath.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/Platform/ARM/Library/BdsLib/BdsFilePath.c b/Platform/ARM/Library/BdsLib/BdsFilePath.c > index 74fdbbee773d..543ac8f83086 100644 > --- a/Platform/ARM/Library/BdsLib/BdsFilePath.c > +++ b/Platform/ARM/Library/BdsLib/BdsFilePath.c > @@ -421,7 +421,7 @@ BdsConnectAndUpdateDevicePath ( > } > > if (RemainingDevicePath) { > - *RemainingDevicePath = Remaining; > + *RemainingDevicePath = DuplicateDevicePath (Remaining); > } > > return Status; OK, so this makes BdsConnectAndUpdateDevicePath()'s RemainingDevicePath output param dynamically allocated. And this change works fine with the recursive logic too, as you say in the commit message. > @@ -1333,14 +1333,18 @@ BdsLoadImageAndUpdateDevicePath ( > } We already need some error handling here. The control flow in BdsConnectAndUpdateDevicePath() boggles my mind a bit, but I think it can output a dynamically allocated RemainingDevicePath *and* return an error. Namely, assume that TryRemovableDevice() is reached, and it fails. So, I think we should add an error handling label ("FreeRemainingDevicePath"), and jump to it, from both first "return" statements in this function. Also, we should likely set RemainingDevicePath to NULL at the top of the function, and check it at the end, because... ugh... BdsConnectAndUpdateDevicePath() might also fail without assigning *RemainingDevicePath? > > FileLoader = FileLoaders; > + Status = EFI_UNSUPPORTED; > while (FileLoader->Support != NULL) { > if (FileLoader->Support (*DevicePath, Handle, RemainingDevicePath)) { > - return FileLoader->LoadImage (DevicePath, Handle, RemainingDevicePath, Type, Image, FileSize); > + Status = FileLoader->LoadImage (DevicePath, Handle, RemainingDevicePath, > + Type, Image, FileSize); > + break; > } > FileLoader++; > } > > - return EFI_UNSUPPORTED; > + FreePool (RemainingDevicePath); > + return Status; > } > > EFI_STATUS > As I mention near the commit message, BdsConnectDevicePath() is not updated. Is that OK? ... Oh wait, BdsConnectDevicePath() is not called by anything. Append another patch to drop it, like BdsStartEfiApplication()? Then this patch will be fine, assuming you add the "goto"s. Thanks! Laszlo