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

[Masahisa Kojima]

On Fri, 10 Nov 2023 at 15:50, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> 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?

Yes, we can set the return value goto err.

> If yes I can fix that during the merge

Thank you. It is very helpful if you can fix this.

Thanks,
Masahisa Kojima

>
> Thanks
> /Ilias