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 C13AC21194865 for ; Thu, 22 Nov 2018 10:09:36 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 459D458E27; Thu, 22 Nov 2018 18:09:36 +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 B186F604CD; Thu, 22 Nov 2018 18:09:31 +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-4-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: <55c8b463-2b30-300f-85e4-5364f96c77c5@redhat.com> Date: Thu, 22 Nov 2018 19:09:30 +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-4-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Thu, 22 Nov 2018 18:09:36 +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:09:36 -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: > 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? Thanks, Laszlo > + > + return Status; > } >