Here is an updated version of the patch. Please bear with me while I learn this email based code review process. I am trying to find resources on this, but there are a lot and I don't know what applies here.
Anyway, I believe there was another leak. If the call to device_bind fails both plat and plat->device_path need to be freed. I think I captured this in the new version. The other open issue was the duplication of efi_dp_size code. But if I understand correctly this was not available in the configuration that we are using. Not sure what the path forward should be here. With kind regards, Janis On Wed, Dec 11, 2024 at 12:23 PM Simon Glass <[email protected]> wrote: > 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 >
From 8956d6d2adad36123df68a93604f19b38b7fa8e7 Mon Sep 17 00:00:00 2001 From: Janis Danisevskis <[email protected]> Date: Sat, 23 Nov 2024 11:55:08 -0800 Subject: [PATCH] Fix efi_bind_block. 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 | 53 ++++++++++++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 12 deletions(-) diff --git a/lib/efi/efi_app_init.c b/lib/efi/efi_app_init.c index 9704020b749..f3ba82280f4 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,27 +48,47 @@ 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 = NULL; 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) - return log_msg_ret("path", -ENOMEM); - memcpy(plat.device_path, device_path, device_path->length); + int ret, err; + 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) { + err = log_msg_ret("path", -ENOMEM); + goto err_out; + } + 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); - if (ret) - return log_msg_ret("bind", ret); + plat, ofnode_null(), &dev); + if (ret) { + err = log_msg_ret("bind", ret); + goto err_out; + } snprintf(name, sizeof(name), "efi_media_%x", dev_seq(dev)); device_set_name(dev, name); *devp = dev; return 0; + +err_out: + if (plat) { + // If plat is not NULL, plat->device_path + // is the return value of malloc. So it is + // either NULL or a valid pointer that needs + // to be freed. + free(plat->device_path); + } + free(plat); + return err; } /** -- 2.34.1
