Hi Heinrich, On Tue, 10 Dec 2024 at 01:50, Heinrich Schuchardt <[email protected]> wrote: > > On 23.11.24 20:55, Matthew Garrett wrote: > > From: Janis Danisevskis <[email protected]> > > > > 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. > > 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 <[email protected]> > > Signed-off-by: Matthew Garrett <[email protected]> > > --- > > > > 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; > > +} > > + > > /** > > * 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) > > 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); > > Here you have a memory leak for pointer plat if ret != 0. > > Simon suggested a follow up patch. Instead we should fix it in this patch. > > @Simon > The logic in the caller setup_block() looks wrong. > > LocateHandle() does not ensure that when you try to read from the block > device the same still does exist. E.g. if a USB device is removed the > block IO protocol could be uninstalled and the handle deleted and the > EFI app may crash. Please, call OpenProtocol() with attribute BY_DRIVER. > > HandleProtocol() is considered deprecated. Please, use OpenProtocol() > instead.
Can you please send a follow-up with your ideas on this? Regards, SImon
