Hi Janis, On Wed, 11 Dec 2024 at 10:50, Janis Danisevskis <[email protected]> wrote: > > 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 :-).
Thanks for the email. If you're able to send a v2, take a look at this series which has a few other things: https://patchwork.ozlabs.org/project/uboot/list/?series=436150 You can incorporate any or all of those and there's no need to keep me as the author of anything in particular. Regards, Simon > > 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
