[PATCH 09/10] Fix efi_bind_block.

[Janis Danisevskis]

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 <sjg at chromium.org> wrote:

> Hi Janis,
>
> On Wed, 11 Dec 2024 at 10:50, Janis Danisevskis
> <jdanisevskis at aurora.tech> 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 <trini at konsulko.com> 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 <xypron.glpk at gmx.de>
> wrote:
> >> > >
> >> > > On 23.11.24 20:55, Matthew Garrett wrote:
> >> > > > From: Janis Danisevskis <jdanisevskis at 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.
> >> > > > 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 <jdanisevskis at aurora.tech>
> >> > > > Signed-off-by: Matthew Garrett <mgarrett at 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;
> >> > > > +}
> >> > > > +
> >> > > >   /**
> >> > > >    * 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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-efi_bind_block.patch
Type: application/x-patch
Size: 2829 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20241211/f61c15b0/attachment.bin>