[PATCH 09/10] Fix efi_bind_block.

[Simon Glass]

Hi Ilias,

On Mon, 25 Nov 2024 at 06:41, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Matthew, Janis,
>
> On Sat, 23 Nov 2024 at 21:57, Matthew Garrett <mjg59 at srcf.ucam.org> 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.
>
> Yep and I wonder how the device_unbind() code managed to survice since
> it free that stack allocated memory...
> Probably never called.

:-)

>
> > 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;
> > +}
>
> We already define efi_dp_size(), you can use that here.

Unfortunately that is only available in EFI_LOADER. Should we try to
split some of the code out to lib/efi ?

>
> > +
> >  /**
> >   * 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)
>
> you need to free plat here now

I will send a patch.

>
> >                 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);
> >
> > --
> > 2.47.0
> >

Regards,
Simon