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 7721321194867 for ; Thu, 22 Nov 2018 10:23:51 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id EFFD7308FC5C; Thu, 22 Nov 2018 18:23:50 +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 4F3976061D; Thu, 22 Nov 2018 18:23:46 +0000 (UTC) To: Ard Biesheuvel Cc: "edk2-devel@lists.01.org" , Leif Lindholm , Thomas Panakamattam Abraham , Nariman Poushin , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= References: <20181122172645.20819-1-ard.biesheuvel@linaro.org> <20181122172645.20819-4-ard.biesheuvel@linaro.org> <55c8b463-2b30-300f-85e4-5364f96c77c5@redhat.com> From: Laszlo Ersek Message-ID: <4c3c0e2d-7978-5248-5280-2ce748058b7c@redhat.com> Date: Thu, 22 Nov 2018 19:23:46 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.43]); Thu, 22 Nov 2018 18:23:51 +0000 (UTC) Subject: Re: [PATCH edk2-platforms 3/4] Platform/ARM/BdsLib: don't clobber BdsLoadImage() DevicePath IN param 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:23:51 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/22/18 19:14, Ard Biesheuvel wrote: > On Thu, 22 Nov 2018 at 19:09, Laszlo Ersek wrote: >> >> On 11/22/18 18:26, Ard Biesheuvel wrote: >>> BdsLoadImage () is part of the BdsLib library API and is not documented >>> as modifying its DevicePath argument, but does so nonetheless. So take >>> a copy instead, and free it after use. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Ard Biesheuvel >>> --- >>> Platform/ARM/Library/BdsLib/BdsFilePath.c | 13 ++++++++++++- >>> 1 file changed, 12 insertions(+), 1 deletion(-) >>> >>> diff --git a/Platform/ARM/Library/BdsLib/BdsFilePath.c b/Platform/ARM/Library/BdsLib/BdsFilePath.c >>> index 67dafa4f3651..74fdbbee773d 100644 >>> --- a/Platform/ARM/Library/BdsLib/BdsFilePath.c >>> +++ b/Platform/ARM/Library/BdsLib/BdsFilePath.c >>> @@ -1351,5 +1351,16 @@ BdsLoadImage ( >>> OUT UINTN *FileSize >>> ) >>> { >>> - return BdsLoadImageAndUpdateDevicePath (&DevicePath, Type, Image, FileSize); >>> + EFI_DEVICE_PATH *Path; >>> + EFI_STATUS Status; >>> + >>> + Path = DuplicateDevicePath (DevicePath); >>> + if (Path == NULL) { >>> + return EFI_OUT_OF_RESOURCES; >>> + } >> >> This introduces a minor change in behavior. >> >> Previously, if BdsLoadImage() got DevicePath==NULL, then >> BdsLoadImageAndUpdateDevicePath() -> BdsConnectAndUpdateDevicePath() >> would hit (*DevicePath == NULL), and return EFI_INVALID_PARAMETER. >> >> Now, (DevicePath==NULL) causes DuplicateDevicePath() to return NULL, and >> we translate that to EFI_OUT_OF_RESOURCES. >> >> Can you check for (DevicePath==NULL) first, and preserve >> EFI_INVALID_PARAMETER? >> >>> + >>> + Status = BdsLoadImageAndUpdateDevicePath (&Path, Type, Image, FileSize); >>> + FreePool (Path); >> >> This is not safe; BdsLoadImageAndUpdateDevicePath() may change Path. >> Namely, in BdsConnectAndUpdateDevicePath(), we have at one location, >> >> *DevicePath = NewDevicePath; >> >> ... Which, in fact, makes me wonder whether we need this patch at all. I >> believe BdsLoadImageAndUpdateDevicePath() -- and >> BdsConnectAndUpdateDevicePath() -- are supposed to update the caller's >> *pointer* to the device path, and not the pointed-to device path itself. >> >> Do you agree? >> > > Indeed. > > EFI_STATUS > BdsLoadImage ( > IN EFI_DEVICE_PATH *DevicePath, > > vs > > EFI_STATUS > BdsLoadImageAndUpdateDevicePath ( > IN OUT EFI_DEVICE_PATH **DevicePath, > > and I didn't spot the diference in * vs ** > > So you are right: BdsConnectAndUpdateDevicePath() assigns to > *DevicePath, which means it updates BdsLoadImage()'s local copy of the > pointer, but not the memory it points to. > > The IN/OUT notation makes this a bit ambiguous, though. Having > something like EFI_DEVICE_PATH CONST ** vs EFI_DEVICE_PATH * CONST * > is not necessarily easier to read, but less ambiguous. > Exactly! Laszlo