[PATCH 09/10] Fix efi_bind_block.

[Ilias Apalodimas]

Hi Janis

On Wed, 11 Dec 2024 at 19:50, Janis Danisevskis
<jdanisevskis at aurora.tech> wrote:
>
> Hi everyone,
>
> Good catch finding this leak. Would you like me to provide an updated patch?

There's a confusion here let me try to explain. I generally think the
patchset is useful and I'd like us to pull it. But we had some review
comments, Simon pulled the patch in his own private tree but it's not
in U-Boot -master or -next -- apologies for any confusion.

If you can go through the comments and let us know, I can work with
you to fix the remaining issues.

Thanks!
/Ilias
> I am good either way. Let Simon take credit for the fix on the git history. I can live with
> shame :-).

We all have our special moments of shame, it's a public ML :)


>
> 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