On Fri, 10 Nov 2023 at 15:50, Ilias Apalodimas
<ilias.apalodi...@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.koj...@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.koj...@linaro.org>
> > Reviewed-by: Ilias Apalodimas <ilias.apalodi...@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
