[PATCH v12 7/9] efi_loader: support boot from URI device path

[Ilias Apalodimas]

Hi Kojima-san

Going through the patch one last time before I merge it I noticed some
memory leaks

On Fri, 10 Nov 2023 at 06:27, Masahisa Kojima
<masahisa.kojima at linaro.org> wrote:
>
> This supports to boot from the URI device path.
> When user selects the URI device path, bootmgr downloads
> the file using wget into the address specified by loadaddr
> env variable.
> If the file is .iso or .img file, mount the image with blkmap
> then try to boot with the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
> Since boot option indicating the default file is automatically
> created when new disk is detected, system can boot by selecting
> the automatically created blkmap boot option.
> If the file is PE-COFF file, load and start the downloaded file.
>
> The buffer used to download the ISO image file must be
> reserved to avoid the unintended access to the image and
> expose the ramdisk to the OS.
> For PE-COFF file case, this memory reservation is done
> in LoadImage Boot Service.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima at linaro.org>
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> ---

[...]

> +
> +/**
> + * efi_bootmgr_image_return_notify() - return to efibootmgr callback
> + *
> + * @event:     the event for which this notification function is registered
> + * @context:   event context
> + */
> +static void EFIAPI efi_bootmgr_image_return_notify(struct efi_event *event,
> +                                                  void *context)
> +{
> +       efi_status_t ret;
> +
> +       EFI_ENTRY("%p, %p", event, context);
> +       ret = efi_bootmgr_release_uridp_resource(context);
> +       EFI_EXIT(ret);
> +}
> +
> +/**
> + * try_load_from_uri_path() - Handle the URI device path
> + *
> + * @uridp:     uri device path
> + * @lo_label:  label of load option
> + * @handle:    pointer to handle for newly installed image
> + * Return:     status code
> + */
> +static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
> +                                          u16 *lo_label,
> +                                          efi_handle_t *handle)
> +{
> +       char *s;
> +       int err;
> +       int uri_len;
> +       efi_status_t ret;
> +       void *source_buffer;
> +       efi_uintn_t source_size;
> +       struct uridp_context *ctx;
> +       struct udevice *blk = NULL;
> +       struct efi_event *event = NULL;
> +       efi_handle_t mem_handle = NULL;
> +       struct efi_device_path *loaded_dp;
> +       static ulong image_size, image_addr;
> +
> +       ctx = calloc(1, sizeof(struct uridp_context));
> +       if (!ctx)
> +               return EFI_OUT_OF_RESOURCES;

ctx is allocated here

> +
> +       s = env_get("loadaddr");
> +       if (!s) {
> +               log_err("Error: loadaddr is not set\n");
> +               return EFI_INVALID_PARAMETER;

Should be freed here

> +       }
> +       image_addr = hextoul(s, NULL);
> +       err = wget_with_dns(image_addr, uridp->uri);
> +       if (err < 0)
> +               return EFI_INVALID_PARAMETER;

and here

> +       image_size = env_get_hex("filesize", 0);
> +       if (!image_size)
> +               return EFI_INVALID_PARAMETER;

and here

> +
> +       /*
> +        * If the file extension is ".iso" or ".img", mount it and try to load
> +        * the default file.
> +        * If the file is PE-COFF image, load the downloaded file.
> +        */
> +       uri_len = strlen(uridp->uri);
> +       if (!strncmp(&uridp->uri[uri_len - 4], ".iso", 4) ||
> +           !strncmp(&uridp->uri[uri_len - 4], ".img", 4)) {
> +               ret = prepare_loaded_image(lo_label, image_addr, image_size,
> +                                          &loaded_dp, &blk);
> +               if (ret != EFI_SUCCESS)
> +                       goto err;
> +
> +               source_buffer = NULL;
> +               source_size = 0;
> +       } else if (efi_check_pe((void *)image_addr, image_size, NULL) == EFI_SUCCESS) {
> +               /*
> +                * loaded_dp must exist until efi application returns,
> +                * will be freed in return_to_efibootmgr event callback.
> +                */
> +               loaded_dp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> +                                           (uintptr_t)image_addr, image_size);
> +               ret = efi_install_multiple_protocol_interfaces(
> +                       &mem_handle, &efi_guid_device_path, loaded_dp, NULL);
> +               if (ret != EFI_SUCCESS)
> +                       goto err;
> +
> +               source_buffer = (void *)image_addr;
> +               source_size = image_size;
> +       } else {
> +               log_err("Error: file type is not supported\n");
> +               return EFI_UNSUPPORTED;

and here

[...]

Should we just goto err and set the return value instead of returning
immediately?
If yes I can fix that during the merge

Thanks
/Ilias