Hi everyone, Good catch finding this leak. Would you like me to provide an updated patch? I am good either way. Let Simon take credit for the fix on the git history. I can live with shame :-).
Janis On Tue, Dec 10, 2024 at 9:11 AM Tom Rini <[email protected]> wrote: > On Tue, Dec 10, 2024 at 09:17:26AM -0700, Simon Glass wrote: > > 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? > > Simon, it's in your tree. The normal path here would be that the > submitter would correct this in v2. But you're trying to short-circuit > that path for your own reasons. No one should be making work on top of > your tree except for you. > > -- > Tom >
