From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 496DEC433F5 for ; Thu, 12 May 2022 07:26:01 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 0EBF983B5C; Thu, 12 May 2022 09:25:58 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="VXCQ7t4+"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 4BC2B84216; Thu, 12 May 2022 09:25:56 +0200 (CEST) Received: from mail-pf1-x42e.google.com (mail-pf1-x42e.google.com [IPv6:2607:f8b0:4864:20::42e]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id C26E180A4F for ; Thu, 12 May 2022 09:25:51 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=takahiro.akashi@linaro.org Received: by mail-pf1-x42e.google.com with SMTP id d25so4018371pfo.10 for ; Thu, 12 May 2022 00:25:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:in-reply-to; bh=2Pg2xJsGWZXCQC4gevii1m+Bi77YKa6b8Z8pqJXcLB4=; b=VXCQ7t4+LMdDERo03OmDUBY8jaHQH/vh/DEGIHg/gbAEZaYFFPnIrto0affGdtVogE QlCOP9w0l78450wNBNTIJ+cm7fcnBOgOj/iIAEToPrdmL7Iji6MpIShaa0KY9jpbu/MB OmL5CPEHyRGhhOf3ildsHy68xUyMF0r5ETsQ9RKFTNwWZ5+OYrrYn4dAnqGd6KYm+Fs4 PjhbYeaKPZxhPaZ11dTZBBMOgFW45DrGdVwRUWkZEv9I75BCK+BxOfKrRHyEn28qkGWY EzKHJ9B2MIXgnM0Q2QvPCOEF5Vvo7ZpehOUyDOo5RTWTOHuxp94PUPFuXkpZFlMLpyzr eFXQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :in-reply-to; bh=2Pg2xJsGWZXCQC4gevii1m+Bi77YKa6b8Z8pqJXcLB4=; b=fCoezzs58cvJ5bi3H2MmrYD3t22Odlmw5qW+3AZUlMciUo1vF/mbugIXTLQGhbyE/d U/TbeQofk3TPBtB6ErbGswOAIKUTaLc6El36hFlI/Q3JMUhRfr/FgZy7zhYUQSIu7wF1 rO/zi4HSTWuSZG4UYn4sakSm9Y/3F+WmWy/2IgCSel/FC5LZ9fcsXn9CX2gMJ+YGwatJ 0JWW6vQsaA+bEJgQM/A+sNA4RS23+WRkItRoSNFMdkbuiWK4kx0QCt3XmcDiSNHTkCLA CbILuU5RizB9O5JKhNjjzjcwTI0O2EdOlf0u6DcDztgzUaUH2oX5Ji9YJ6wDp2ExIDLC zDIw== X-Gm-Message-State: AOAM530RQ0TACXMtF7VLVnrBukkzIoCLBSSHHGCNj1eaGc6BE7QcyM7X u+Dr1STo3qCtSaPNE3KaMRplZw== X-Google-Smtp-Source: ABdhPJz4EbQ00vuC0tR1zruOVHCer8jar1cj0h7mgOHtHHrGckVQpujU51pS3W15fyt7RTfoByGGcA== X-Received: by 2002:a05:6a00:181d:b0:50d:d56c:73d3 with SMTP id y29-20020a056a00181d00b0050dd56c73d3mr28758939pfa.22.1652340350085; Thu, 12 May 2022 00:25:50 -0700 (PDT) Received: from laputa ([2400:4050:c3e1:100:f5e6:8e26:8fc:a1db]) by smtp.gmail.com with ESMTPSA id m7-20020a170902f64700b0015e8d4eb1e1sm3166157plg.43.2022.05.12.00.25.48 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 12 May 2022 00:25:49 -0700 (PDT) Date: Thu, 12 May 2022 16:25:46 +0900 From: AKASHI Takahiro To: Heinrich Schuchardt Cc: mark.kettenis@xs4all.nl, u-boot@lists.denx.de Subject: Re: [PATCH v2 2/3] efi_loader: bootmgr: fix a problem in loading an image from a short-path Message-ID: <20220512072546.GB88188@laputa> Mail-Followup-To: AKASHI Takahiro , Heinrich Schuchardt , mark.kettenis@xs4all.nl, u-boot@lists.denx.de References: <20220512022903.65346-1-takahiro.akashi@linaro.org> <20220512022903.65346-3-takahiro.akashi@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean On Thu, May 12, 2022 at 08:50:34AM +0200, Heinrich Schuchardt wrote: > On 5/12/22 04:29, AKASHI Takahiro wrote: > > Booting from a short-form device path which starts with the first element > > being a File Path Media Device Path failed because it doesn't contain > > any valid device with simple file system protocol and efi_dp_find_obj() > > in efi_load_image_from_path() will return NULL. > > For instance, > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)/\helloworld.efi > > -> shortened version: /\helloworld.efi > > > > With this patch applied, all the media devices with simple file system > > protocol are enumerated and the boot manager attempts to boot temporarily > > generated device paths one-by-one. > > > > This new implementation is still a bit incompatible with the UEFI > > specification in terms of: > > * not creating real boot options > > * not try > > "If a device does not support the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, but > > supports the EFI_BLOCK_IO_PROTOCOL protocol, then the EFI Boot Service > > ConnectController must be called for this device with DriverImageHandle > > and RemainingDevicePath set to NULL and the Recursive flag is set to TRUE." > > We already have the ConnectController() logic in lib/efi_driver/. We > just have to carry it to all block devices (in a future patch set). > > > (See section 3.1.2 "Load Option Processing".) > > > > But it still gives us a closer and better solution than the current. > > > > Fixes: commit 9cdf470274ff ("efi_loader: support booting via short-form device-path") > > Signed-off-by: AKASHI Takahiro > > --- > > lib/efi_loader/efi_bootmgr.c | 98 ++++++++++++++++++++++++++++++++++-- > > 1 file changed, 94 insertions(+), 4 deletions(-) > > > > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c > > index 631a25d76e9e..3608e433503e 100644 > > --- a/lib/efi_loader/efi_bootmgr.c > > +++ b/lib/efi_loader/efi_bootmgr.c > > @@ -76,6 +76,91 @@ struct efi_device_path *expand_media_path(struct efi_device_path *device_path) > > return full_path; > > } > > > > The logic in this patch looks good to me. Just some nits: > > Please, provide a description of the function and its parameters. I don't think we need a description here because, as you can see, __try_load() is an inner function and intended to be called only from try_load_from_short_path(). The purpose of extracting it is to avoid a duplicated code to go though a list of handles *twice*, for removable media and for fixed media. The description of try_load_from_short_path() covers both. > > +static efi_status_t __try_load(efi_handle_t *fs_handles, efi_uintn_t num, > > Please, use a name not starting with underscores conveying what the > function is good for, e.g. efi_load_from_file_path(). > > > + struct efi_device_path *fp, > > + efi_handle_t *handle, bool removable) > > +{ > > + struct efi_handler *handler; > > + struct efi_device_path *dp; > > + int i; > > + efi_status_t ret; > > + > > + for (i = 0; i < num; i++) { > > + if (removable && !efi_disk_is_removable(fs_handles[i])) > > + continue; > > + if (!removable && efi_disk_is_removable(fs_handles[i])) > > + continue; > > + > > + ret = efi_search_protocol(fs_handles[i], &efi_guid_device_path, > > + &handler); > > + if (ret != EFI_SUCCESS) > > + /* unlikely */ > > If you want to tell the compiler that this path is unlikely: > > if (unlikely(ret != EFI_SUCCESS)) > > But as this code is not called often I would simply remove the comment. > > > + continue; > > + > > + dp = handler->protocol_interface; > > + if (!dp) > > + /* unlikely */ > > ditto > > > + continue; > > + > > + dp = efi_dp_append(dp, fp); > > You are leaking the original dp here. Please, avoid memory leaks. Actually, not. The original dp comes from "handler->protocol_interface" and it should exist while the associated handle, (fs_handles[i]), exists. -Takahiro Akashi > > > + if (!dp) > > + /* unlikely */ > > if (unlikely( ? > > > + continue; > > + > > + ret = EFI_CALL(efi_load_image(true, efi_root, dp, NULL, 0, > > + handle)); > > + efi_free_pool(dp); > > + if (ret == EFI_SUCCESS) > > + return ret; > > + } > > + > > + return EFI_NOT_FOUND; > > +} > > + > > +/** > > + * try_load_from_short_path > > + * @fp: file path > > + * @handle: pointer to handle for newly installed image > > + * > > + * Enumerate all the devices which support file system operations, > > + * prepend its media device path to the file path, @fp, and > > + * try to load the file. > > + * This function should be called when handling a short-form path > > + * which is starting with a file device path. > > + * > > + * Return: status code > > + */ > > +static efi_status_t try_load_from_short_path(struct efi_device_path *fp, > > + efi_handle_t *handle) > > +{ > > + efi_handle_t *fs_handles; > > + efi_uintn_t num; > > + efi_status_t ret; > > + > > + ret = EFI_CALL(efi_locate_handle_buffer( > > + BY_PROTOCOL, > > + &efi_simple_file_system_protocol_guid, > > + NULL, > > + &num, &fs_handles)); > > + if (ret != EFI_SUCCESS) > > + return ret; > > + if (!num) > > + return EFI_NOT_FOUND; > > + > > + /* removable media first */ > > + ret = __try_load(fs_handles, num, fp, handle, true); > > + if (ret == EFI_SUCCESS) > > + goto out; > > + > > + /* fixed media */ > > + ret = __try_load(fs_handles, num, fp, handle, false); > > + if (ret == EFI_SUCCESS) > > + goto out; > > + > > +out: > > + return ret; > > +} > > + > > /** > > * try_load_entry() - try to load image for boot option > > * > > @@ -116,10 +201,15 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle, > > log_debug("trying to load \"%ls\" from %pD\n", lo.label, > > lo.file_path); > > > > - file_path = expand_media_path(lo.file_path); > > - ret = EFI_CALL(efi_load_image(true, efi_root, file_path, > > - NULL, 0, handle)); > > - efi_free_pool(file_path); > > + if (EFI_DP_TYPE(lo.file_path, MEDIA_DEVICE, FILE_PATH)) { > > + /* file_path doesn't contain a device path */ > > %s/device path/device part/ > > In UEFI speak a file_path is a device path. > > Best regards > > Heinrich > > > + ret = try_load_from_short_path(lo.file_path, handle); > > + } else { > > + file_path = expand_media_path(lo.file_path); > > + ret = EFI_CALL(efi_load_image(true, efi_root, file_path, > > + NULL, 0, handle)); > > + efi_free_pool(file_path); > > + } > > if (ret != EFI_SUCCESS) { > > log_warning("Loading %ls '%ls' failed\n", > > varname, lo.label);