From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::143; helo=mail-it1-x143.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it1-x143.google.com (mail-it1-x143.google.com [IPv6:2607:f8b0:4864:20::143]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 7C0C121194867 for ; Thu, 22 Nov 2018 10:15:06 -0800 (PST) Received: by mail-it1-x143.google.com with SMTP id x19so15245318itl.1 for ; Thu, 22 Nov 2018 10:15:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=TZLN+IHI6Ygaji/TqsKAn4o49RkqhrPvYtYVN9vupf8=; b=BG9tseaEhCAZAVhwOGwaepGT9AulY+U2+3sy7AtRYZI9qKVbW+4hlOwOEU4Wd4tMjr 9lQLkzwjftDY5XhkdNchX7KVs9GkUIJXgB6u7HaBbt9VShDVqP5jDZMuCkzhBC3fDF+8 r7BcpHidabCDTs6+hfJfeJbnVUV1tWg0VPfcM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=TZLN+IHI6Ygaji/TqsKAn4o49RkqhrPvYtYVN9vupf8=; b=patvmhpAONNIsQkW5EbT+9u/EiF9WMChtvF1WCPtJTY98D/EcHTN8i9k8LkKYJ8XOO h3sNoCTqXre7kkK5YyGthVGSg0U30x8V7R7mt3izDZM93wHAsJlN85HaPtACv3WOFs01 RHxOx6vHaDdvdE69R89vra7IVqad9MOM0tmDvh2aGA5BNh34R7KJmgSBM28gaVGBcvKY 3aZ80fIXkP0LAiMdijD2PJXqVjGo/ftF0Qk6jIZLfBXB7r5lxx/do9914pqKJTj4nrvi 94kff8oi7krwiOJsUFkijedP9QskcYVR8dOnj1DNbz0JqoYhkBHWLJ0532CitenbwR+m +uWg== X-Gm-Message-State: AGRZ1gKshCwTK862MTzIZv5+8lQw6VGEBUYkq37Vz/ek36AxGH9MZU6R ItyAUFCvJLasFLttB4Me0HJTjnwXDrD+AO3cbQh+hw== X-Google-Smtp-Source: AJdET5doksVc1FjqtrZQoppgO/877ZabfIWUFwhqAE501NJuhNo30h8+tVRkqWYLyiphdmudSHn0nZEx0m0kylg7vV0= X-Received: by 2002:a05:660c:4b:: with SMTP id p11mr10760854itk.71.1542910505190; Thu, 22 Nov 2018 10:15:05 -0800 (PST) MIME-Version: 1.0 References: <20181122172645.20819-1-ard.biesheuvel@linaro.org> <20181122172645.20819-4-ard.biesheuvel@linaro.org> <55c8b463-2b30-300f-85e4-5364f96c77c5@redhat.com> In-Reply-To: <55c8b463-2b30-300f-85e4-5364f96c77c5@redhat.com> From: Ard Biesheuvel Date: Thu, 22 Nov 2018 19:14:53 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Leif Lindholm , Thomas Panakamattam Abraham , Nariman Poushin , =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= 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:15:06 -0000 Content-Type: text/plain; charset="UTF-8" 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.