[PATCH 09/10] Fix efi_bind_block.

[Simon Glass]

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