Re: [PATCH 2/2] efi/gop: Fix memory leak in __gop_query32/64

[Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>]

On Wed, 4 Dec 2019 at 15:27, Arvind Sankar <nivedita@xxxxxxxxxxxx> wrote:
>
> On Wed, Dec 04, 2019 at 03:11:09PM +0000, Ard Biesheuvel wrote:
> > On Tue, 3 Dec 2019 at 21:47, Arvind Sankar <nivedita@xxxxxxxxxxxx> wrote:
> > >
> > > gop->query_mode returns info in callee-allocated memory which must be
> > > freed by the caller.
> > >
> > > We don't actually need to call it in order to obtain the info for the
> > > current graphics mode, which is already there in gop->mode->info, so
> > > just access it directly.
> > >
> > > Also nothing uses the size of the info structure, so remove the
> > > argument.
> > >
> > > Signed-off-by: Arvind Sankar <nivedita@xxxxxxxxxxxx>
> >
> > Thanks Arvind
> >
> > I agree with this patch in principle, but I'd prefer it if we could
> > get rid of the __gop_queryXX routines altogether, or if we need a
> > helper, to at least merge them into on, taking gopXX->mode as an input
> > argument.
> >
>
> I can do that. I'm also planning a few patches later to merge the 32-bit
> and 64-bit versions together if there are no objections, but that needs
> a little more work. Right now the query code can't be merged together
> because mode's layout is different between 32-bit and 64-bit versions,
> but it can certainly be folded into the main setup routines.
>

Fair enough.

Are you building/testing this on x86 hardware only? That's perfectly
fine, but it would be good to know.

> >
> > > ---
> > >  drivers/firmware/efi/libstub/gop.c | 48 ++++++++----------------------
> > >  1 file changed, 12 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c
> > > index 235a98797105..c8a39cd89b47 100644
> > > --- a/drivers/firmware/efi/libstub/gop.c
> > > +++ b/drivers/firmware/efi/libstub/gop.c
> > > @@ -83,28 +83,17 @@ setup_pixel_info(struct screen_info *si, u32 pixels_per_scan_line,
> > >         }
> > >  }
> > >
> > > -static efi_status_t
> > > +static void
> > >  __gop_query32(efi_system_table_t *sys_table_arg,
> > >               struct efi_graphics_output_protocol_32 *gop32,
> > >               struct efi_graphics_output_mode_info **info,
> > > -             unsigned long *size, u64 *fb_base)
> > > +             u64 *fb_base)
> > >  {
> > >         struct efi_graphics_output_protocol_mode_32 *mode;
> > > -       efi_graphics_output_protocol_query_mode query_mode;
> > > -       efi_status_t status;
> > > -       unsigned long m;
> > > -
> > > -       m = gop32->mode;
> > > -       mode = (struct efi_graphics_output_protocol_mode_32 *)m;
> > > -       query_mode = (void *)(unsigned long)gop32->query_mode;
> > > -
> > > -       status = __efi_call_early(query_mode, (void *)gop32, mode->mode, size,
> > > -                                 info);
> > > -       if (status != EFI_SUCCESS)
> > > -               return status;
> > >
> > > +       mode = (void *)(unsigned long)gop32->mode;
> > > +       *info = (void *)(unsigned long)mode->info;
> > >         *fb_base = mode->frame_buffer_base;
> > > -       return status;
> > >  }
> > >
> > >  static efi_status_t
> > > @@ -145,9 +134,8 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
> > >                 if (status == EFI_SUCCESS)
> > >                         conout_found = true;
> > >
> > > -               status = __gop_query32(sys_table_arg, gop32, &info, &size,
> > > -                                      &current_fb_base);
> > > -               if (status == EFI_SUCCESS && (!first_gop || conout_found) &&
> > > +               __gop_query32(sys_table_arg, gop32, &info, &current_fb_base);
> > > +               if ((!first_gop || conout_found) &&
> > >                     info->pixel_format != PIXEL_BLT_ONLY) {
> > >                         /*
> > >                          * Systems that use the UEFI Console Splitter may
> > > @@ -201,28 +189,17 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
> > >         return EFI_SUCCESS;
> > >  }
> > >
> > > -static efi_status_t
> > > +static void
> > >  __gop_query64(efi_system_table_t *sys_table_arg,
> > >               struct efi_graphics_output_protocol_64 *gop64,
> > >               struct efi_graphics_output_mode_info **info,
> > > -             unsigned long *size, u64 *fb_base)
> > > +             u64 *fb_base)
> > >  {
> > >         struct efi_graphics_output_protocol_mode_64 *mode;
> > > -       efi_graphics_output_protocol_query_mode query_mode;
> > > -       efi_status_t status;
> > > -       unsigned long m;
> > > -
> > > -       m = gop64->mode;
> > > -       mode = (struct efi_graphics_output_protocol_mode_64 *)m;
> > > -       query_mode = (void *)(unsigned long)gop64->query_mode;
> > > -
> > > -       status = __efi_call_early(query_mode, (void *)gop64, mode->mode, size,
> > > -                                 info);
> > > -       if (status != EFI_SUCCESS)
> > > -               return status;
> > >
> > > +       mode = (void *)(unsigned long)gop64->mode;
> > > +       *info = (void *)(unsigned long)mode->info;
> > >         *fb_base = mode->frame_buffer_base;
> > > -       return status;
> > >  }
> > >
> > >  static efi_status_t
> > > @@ -263,9 +240,8 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
> > >                 if (status == EFI_SUCCESS)
> > >                         conout_found = true;
> > >
> > > -               status = __gop_query64(sys_table_arg, gop64, &info, &size,
> > > -                                      &current_fb_base);
> > > -               if (status == EFI_SUCCESS && (!first_gop || conout_found) &&
> > > +               __gop_query64(sys_table_arg, gop64, &info, &current_fb_base);
> > > +               if ((!first_gop || conout_found) &&
> > >                     info->pixel_format != PIXEL_BLT_ONLY) {
> > >                         /*
> > >                          * Systems that use the UEFI Console Splitter may
> > > --
> > > 2.23.0
> > >