Hi Matthew, Janis,
On Sat, 23 Nov 2024 at 21:57, Matthew Garrett <mj...@srcf.ucam.org> wrote:
>
> From: Janis Danisevskis <jdanisevs...@aurora.tech>
>
> efi_bind_block had two issues.
> 1. A pointer to a the stack was inserted as plat structure and thus used
> beyond its life time.
Yep and I wonder how the device_unbind() code managed to survice since
it free that stack allocated memory...
Probably never called.
> 2. Only the first segment of the device path was copied into the
> platfom data structure resulting in an unterminated device path.
>
> Signed-off-by: Janis Danisevskis <jdanisevs...@aurora.tech>
> Signed-off-by: Matthew Garrett <mgarr...@aurora.tech>
> ---
>
> lib/efi/efi_app_init.c | 29 +++++++++++++++++++++--------
> 1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/lib/efi/efi_app_init.c b/lib/efi/efi_app_init.c
> index 9704020b749..cc91e1d74b8 100644
> --- a/lib/efi/efi_app_init.c
> +++ b/lib/efi/efi_app_init.c
> @@ -19,6 +19,15 @@
>
> DECLARE_GLOBAL_DATA_PTR;
>
> +static size_t device_path_length(const struct efi_device_path *device_path)
> +{
> + const struct efi_device_path *d;
> +
> + for (d = device_path; d->type != DEVICE_PATH_TYPE_END; d = (void *)d
> + d->length) {
> + }
> + return (void *)d - (void *)device_path + d->length;
> +}
We already define efi_dp_size(), you can use that here.
> +
> /**
> * efi_bind_block() - bind a new block device to an EFI device
> *
> @@ -39,19 +48,23 @@ int efi_bind_block(efi_handle_t handle, struct
> efi_block_io *blkio,
> struct efi_device_path *device_path, int len,
> struct udevice **devp)
> {
> - struct efi_media_plat plat;
> + struct efi_media_plat *plat;
> struct udevice *dev;
> char name[18];
> int ret;
> -
> - plat.handle = handle;
> - plat.blkio = blkio;
> - plat.device_path = malloc(device_path->length);
> - if (!plat.device_path)
> + size_t device_path_len = device_path_length(device_path);
> +
> + plat = malloc(sizeof(struct efi_media_plat));
> + if (!plat)
> + return log_msg_ret("plat", -ENOMEM);
> + plat->handle = handle;
> + plat->blkio = blkio;
> + plat->device_path = malloc(device_path_len);
> + if (!plat->device_path)
you need to free plat here now
> return log_msg_ret("path", -ENOMEM);
> - memcpy(plat.device_path, device_path, device_path->length);
> + memcpy(plat->device_path, device_path, device_path_len);
> ret = device_bind(dm_root(), DM_DRIVER_GET(efi_media), "efi_media",
> - &plat, ofnode_null(), &dev);
> + plat, ofnode_null(), &dev);
> if (ret)
> return log_msg_ret("bind", ret);
>
> --
> 2.47.0
>
Thanks
/Ilias
