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

[Arvind Sankar <nivedita@xxxxxxxxxxxx>]

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.

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