[PATCH 1/2] efi/gop: Fix return value of setup_gop32/64

[PATCH 1/2] efi/gop: Fix return value of setup_gop32/64

[Arvind Sankar]

There are two ways the status return of setup_gop32/64 could be
incorrect.

1. If we don't find a usable GOP because none of them have a framebuffer
(i.e. they were all PIXEL_BLT_ONLY), but all the efi calls succeeded, we
will return EFI_SUCCESS even though we didn't find a usable GOP. Return
EFI_NOT_FOUND instead, allowing the caller to probe for UGA instead.

2. If we do find a usable GOP, but one of the subsequent ones fails in
an EFI call while checking if any support console output, we may return
an EFI error code even though we found a usable GOP. Fix this by always
returning EFI_SUCCESS if we got a usable GOP.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 drivers/firmware/efi/libstub/gop.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c
index 0101ca4c13b1..235a98797105 100644
--- a/drivers/firmware/efi/libstub/gop.c
+++ b/drivers/firmware/efi/libstub/gop.c
@@ -119,7 +119,6 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
 	u64 fb_base;
 	struct efi_pixel_bitmask pixel_info;
 	int pixel_format;
-	efi_status_t status = EFI_NOT_FOUND;
 	u32 *handles = (u32 *)(unsigned long)gop_handle;
 	int i;
 
@@ -134,6 +133,7 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
 		void *dummy = NULL;
 		efi_handle_t h = (efi_handle_t)(unsigned long)handles[i];
 		u64 current_fb_base;
+		efi_status_t status;
 
 		status = efi_call_early(handle_protocol, h,
 					proto, (void **)&gop32);
@@ -175,7 +175,7 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
 
 	/* Did we find any GOPs? */
 	if (!first_gop)
-		goto out;
+		return EFI_NOT_FOUND;
 
 	/* EFI framebuffer */
 	si->orig_video_isVGA = VIDEO_TYPE_EFI;
@@ -197,8 +197,8 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
 	si->lfb_size = si->lfb_linelength * si->lfb_height;
 
 	si->capabilities |= VIDEO_CAPABILITY_SKIP_QUIRKS;
-out:
-	return status;
+
+	return EFI_SUCCESS;
 }
 
 static efi_status_t
@@ -237,7 +237,6 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
 	u64 fb_base;
 	struct efi_pixel_bitmask pixel_info;
 	int pixel_format;
-	efi_status_t status = EFI_NOT_FOUND;
 	u64 *handles = (u64 *)(unsigned long)gop_handle;
 	int i;
 
@@ -252,6 +251,7 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
 		void *dummy = NULL;
 		efi_handle_t h = (efi_handle_t)(unsigned long)handles[i];
 		u64 current_fb_base;
+		efi_status_t status;
 
 		status = efi_call_early(handle_protocol, h,
 					proto, (void **)&gop64);
@@ -293,7 +293,7 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
 
 	/* Did we find any GOPs? */
 	if (!first_gop)
-		goto out;
+		return EFI_NOT_FOUND;
 
 	/* EFI framebuffer */
 	si->orig_video_isVGA = VIDEO_TYPE_EFI;
@@ -315,8 +315,8 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
 	si->lfb_size = si->lfb_linelength * si->lfb_height;
 
 	si->capabilities |= VIDEO_CAPABILITY_SKIP_QUIRKS;
-out:
-	return status;
+
+	return EFI_SUCCESS;
 }
 
 /*
-- 
2.23.0

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

[Arvind Sankar]

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@alum.mit.edu>
---
 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

Re: [PATCH 1/2] efi/gop: Fix return value of setup_gop32/64

[Ard Biesheuvel]

On Tue, 3 Dec 2019 at 21:47, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> There are two ways the status return of setup_gop32/64 could be
> incorrect.
>
> 1. If we don't find a usable GOP because none of them have a framebuffer
> (i.e. they were all PIXEL_BLT_ONLY), but all the efi calls succeeded, we
> will return EFI_SUCCESS even though we didn't find a usable GOP. Return
> EFI_NOT_FOUND instead, allowing the caller to probe for UGA instead.
>
> 2. If we do find a usable GOP, but one of the subsequent ones fails in
> an EFI call while checking if any support console output, we may return
> an EFI error code even though we found a usable GOP. Fix this by always
> returning EFI_SUCCESS if we got a usable GOP.
>

Please split this into 2 patches

> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> ---
>  drivers/firmware/efi/libstub/gop.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c
> index 0101ca4c13b1..235a98797105 100644
> --- a/drivers/firmware/efi/libstub/gop.c
> +++ b/drivers/firmware/efi/libstub/gop.c
> @@ -119,7 +119,6 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
>         u64 fb_base;
>         struct efi_pixel_bitmask pixel_info;
>         int pixel_format;
> -       efi_status_t status = EFI_NOT_FOUND;

Is it really necessary to move this declaration?

>         u32 *handles = (u32 *)(unsigned long)gop_handle;
>         int i;
>
> @@ -134,6 +133,7 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
>                 void *dummy = NULL;
>                 efi_handle_t h = (efi_handle_t)(unsigned long)handles[i];
>                 u64 current_fb_base;
> +               efi_status_t status;
>
>                 status = efi_call_early(handle_protocol, h,
>                                         proto, (void **)&gop32);
> @@ -175,7 +175,7 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
>
>         /* Did we find any GOPs? */
>         if (!first_gop)
> -               goto out;
> +               return EFI_NOT_FOUND;
>
>         /* EFI framebuffer */
>         si->orig_video_isVGA = VIDEO_TYPE_EFI;
> @@ -197,8 +197,8 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
>         si->lfb_size = si->lfb_linelength * si->lfb_height;
>
>         si->capabilities |= VIDEO_CAPABILITY_SKIP_QUIRKS;
> -out:
> -       return status;
> +
> +       return EFI_SUCCESS;
>  }
>
>  static efi_status_t
> @@ -237,7 +237,6 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
>         u64 fb_base;
>         struct efi_pixel_bitmask pixel_info;
>         int pixel_format;
> -       efi_status_t status = EFI_NOT_FOUND;
>         u64 *handles = (u64 *)(unsigned long)gop_handle;
>         int i;
>
> @@ -252,6 +251,7 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
>                 void *dummy = NULL;
>                 efi_handle_t h = (efi_handle_t)(unsigned long)handles[i];
>                 u64 current_fb_base;
> +               efi_status_t status;
>
>                 status = efi_call_early(handle_protocol, h,
>                                         proto, (void **)&gop64);
> @@ -293,7 +293,7 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
>
>         /* Did we find any GOPs? */
>         if (!first_gop)
> -               goto out;
> +               return EFI_NOT_FOUND;
>
>         /* EFI framebuffer */
>         si->orig_video_isVGA = VIDEO_TYPE_EFI;
> @@ -315,8 +315,8 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
>         si->lfb_size = si->lfb_linelength * si->lfb_height;
>
>         si->capabilities |= VIDEO_CAPABILITY_SKIP_QUIRKS;
> -out:
> -       return status;
> +
> +       return EFI_SUCCESS;
>  }
>
>  /*
> --
> 2.23.0
>

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

[Ard Biesheuvel]

On Tue, 3 Dec 2019 at 21:47, Arvind Sankar <nivedita@alum.mit.edu> 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@alum.mit.edu>

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.


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

Re: [PATCH 1/2] efi/gop: Fix return value of setup_gop32/64

[Arvind Sankar]

On Wed, Dec 04, 2019 at 03:03:04PM +0000, Ard Biesheuvel wrote:
> On Tue, 3 Dec 2019 at 21:47, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > There are two ways the status return of setup_gop32/64 could be
> > incorrect.
> >
> > 1. If we don't find a usable GOP because none of them have a framebuffer
> > (i.e. they were all PIXEL_BLT_ONLY), but all the efi calls succeeded, we
> > will return EFI_SUCCESS even though we didn't find a usable GOP. Return
> > EFI_NOT_FOUND instead, allowing the caller to probe for UGA instead.
> >
> > 2. If we do find a usable GOP, but one of the subsequent ones fails in
> > an EFI call while checking if any support console output, we may return
> > an EFI error code even though we found a usable GOP. Fix this by always
> > returning EFI_SUCCESS if we got a usable GOP.
> >
> 
> Please split this into 2 patches

Not quite following -- what should be the two pieces? Are you saying to
split the success return and the failure return into two patches -- that
seems excessively fine-grained.

> 
> > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> > ---
> >  drivers/firmware/efi/libstub/gop.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c
> > index 0101ca4c13b1..235a98797105 100644
> > --- a/drivers/firmware/efi/libstub/gop.c
> > +++ b/drivers/firmware/efi/libstub/gop.c
> > @@ -119,7 +119,6 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
> >         u64 fb_base;
> >         struct efi_pixel_bitmask pixel_info;
> >         int pixel_format;
> > -       efi_status_t status = EFI_NOT_FOUND;
> 
> Is it really necessary to move this declaration?

No, I just moved it inside the loop block since it's not used outside
any more.

> 
> >         u32 *handles = (u32 *)(unsigned long)gop_handle;
> >         int i;
> >
> > @@ -134,6 +133,7 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
> >                 void *dummy = NULL;
> >                 efi_handle_t h = (efi_handle_t)(unsigned long)handles[i];
> >                 u64 current_fb_base;
> > +               efi_status_t status;
> >
> >                 status = efi_call_early(handle_protocol, h,
> >                                         proto, (void **)&gop32);
> > @@ -175,7 +175,7 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
> >
> >         /* Did we find any GOPs? */
> >         if (!first_gop)
> > -               goto out;
> > +               return EFI_NOT_FOUND;
> >
> >         /* EFI framebuffer */
> >         si->orig_video_isVGA = VIDEO_TYPE_EFI;
> > @@ -197,8 +197,8 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
> >         si->lfb_size = si->lfb_linelength * si->lfb_height;
> >
> >         si->capabilities |= VIDEO_CAPABILITY_SKIP_QUIRKS;
> > -out:
> > -       return status;
> > +
> > +       return EFI_SUCCESS;
> >  }
> >
> >  static efi_status_t
> > @@ -237,7 +237,6 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
> >         u64 fb_base;
> >         struct efi_pixel_bitmask pixel_info;
> >         int pixel_format;
> > -       efi_status_t status = EFI_NOT_FOUND;
> >         u64 *handles = (u64 *)(unsigned long)gop_handle;
> >         int i;
> >
> > @@ -252,6 +251,7 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
> >                 void *dummy = NULL;
> >                 efi_handle_t h = (efi_handle_t)(unsigned long)handles[i];
> >                 u64 current_fb_base;
> > +               efi_status_t status;
> >
> >                 status = efi_call_early(handle_protocol, h,
> >                                         proto, (void **)&gop64);
> > @@ -293,7 +293,7 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
> >
> >         /* Did we find any GOPs? */
> >         if (!first_gop)
> > -               goto out;
> > +               return EFI_NOT_FOUND;
> >
> >         /* EFI framebuffer */
> >         si->orig_video_isVGA = VIDEO_TYPE_EFI;
> > @@ -315,8 +315,8 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
> >         si->lfb_size = si->lfb_linelength * si->lfb_height;
> >
> >         si->capabilities |= VIDEO_CAPABILITY_SKIP_QUIRKS;
> > -out:
> > -       return status;
> > +
> > +       return EFI_SUCCESS;
> >  }
> >
> >  /*
> > --
> > 2.23.0
> >

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

[Arvind Sankar]

On Wed, Dec 04, 2019 at 03:11:09PM +0000, Ard Biesheuvel wrote:
> On Tue, 3 Dec 2019 at 21:47, Arvind Sankar <nivedita@alum.mit.edu> 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@alum.mit.edu>
> 
> 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
> >

Re: [PATCH 1/2] efi/gop: Fix return value of setup_gop32/64

[Ard Biesheuvel]

On Wed, 4 Dec 2019 at 15:23, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Wed, Dec 04, 2019 at 03:03:04PM +0000, Ard Biesheuvel wrote:
> > On Tue, 3 Dec 2019 at 21:47, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > There are two ways the status return of setup_gop32/64 could be
> > > incorrect.
> > >
> > > 1. If we don't find a usable GOP because none of them have a framebuffer
> > > (i.e. they were all PIXEL_BLT_ONLY), but all the efi calls succeeded, we
> > > will return EFI_SUCCESS even though we didn't find a usable GOP. Return
> > > EFI_NOT_FOUND instead, allowing the caller to probe for UGA instead.
> > >
> > > 2. If we do find a usable GOP, but one of the subsequent ones fails in
> > > an EFI call while checking if any support console output, we may return
> > > an EFI error code even though we found a usable GOP. Fix this by always
> > > returning EFI_SUCCESS if we got a usable GOP.
> > >
> >
> > Please split this into 2 patches
>
> Not quite following -- what should be the two pieces? Are you saying to
> split the success return and the failure return into two patches -- that
> seems excessively fine-grained.
>

Your commit log describes two distinct changes 1. and 2. The patch
addresses those two issues, and in addition, moves variable
declarations around. The result is much more difficult to read than
necessary.

> >
> > > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> > > ---
> > >  drivers/firmware/efi/libstub/gop.c | 16 ++++++++--------
> > >  1 file changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c
> > > index 0101ca4c13b1..235a98797105 100644
> > > --- a/drivers/firmware/efi/libstub/gop.c
> > > +++ b/drivers/firmware/efi/libstub/gop.c
> > > @@ -119,7 +119,6 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
> > >         u64 fb_base;
> > >         struct efi_pixel_bitmask pixel_info;
> > >         int pixel_format;
> > > -       efi_status_t status = EFI_NOT_FOUND;
> >
> > Is it really necessary to move this declaration?
>
> No, I just moved it inside the loop block since it's not used outside
> any more.
>
> >
> > >         u32 *handles = (u32 *)(unsigned long)gop_handle;
> > >         int i;
> > >
> > > @@ -134,6 +133,7 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
> > >                 void *dummy = NULL;
> > >                 efi_handle_t h = (efi_handle_t)(unsigned long)handles[i];
> > >                 u64 current_fb_base;
> > > +               efi_status_t status;
> > >
> > >                 status = efi_call_early(handle_protocol, h,
> > >                                         proto, (void **)&gop32);
> > > @@ -175,7 +175,7 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
> > >
> > >         /* Did we find any GOPs? */
> > >         if (!first_gop)
> > > -               goto out;
> > > +               return EFI_NOT_FOUND;
> > >
> > >         /* EFI framebuffer */
> > >         si->orig_video_isVGA = VIDEO_TYPE_EFI;
> > > @@ -197,8 +197,8 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
> > >         si->lfb_size = si->lfb_linelength * si->lfb_height;
> > >
> > >         si->capabilities |= VIDEO_CAPABILITY_SKIP_QUIRKS;
> > > -out:
> > > -       return status;
> > > +
> > > +       return EFI_SUCCESS;
> > >  }
> > >
> > >  static efi_status_t
> > > @@ -237,7 +237,6 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
> > >         u64 fb_base;
> > >         struct efi_pixel_bitmask pixel_info;
> > >         int pixel_format;
> > > -       efi_status_t status = EFI_NOT_FOUND;
> > >         u64 *handles = (u64 *)(unsigned long)gop_handle;
> > >         int i;
> > >
> > > @@ -252,6 +251,7 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
> > >                 void *dummy = NULL;
> > >                 efi_handle_t h = (efi_handle_t)(unsigned long)handles[i];
> > >                 u64 current_fb_base;
> > > +               efi_status_t status;
> > >
> > >                 status = efi_call_early(handle_protocol, h,
> > >                                         proto, (void **)&gop64);
> > > @@ -293,7 +293,7 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
> > >
> > >         /* Did we find any GOPs? */
> > >         if (!first_gop)
> > > -               goto out;
> > > +               return EFI_NOT_FOUND;
> > >
> > >         /* EFI framebuffer */
> > >         si->orig_video_isVGA = VIDEO_TYPE_EFI;
> > > @@ -315,8 +315,8 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
> > >         si->lfb_size = si->lfb_linelength * si->lfb_height;
> > >
> > >         si->capabilities |= VIDEO_CAPABILITY_SKIP_QUIRKS;
> > > -out:
> > > -       return status;
> > > +
> > > +       return EFI_SUCCESS;
> > >  }
> > >
> > >  /*
> > > --
> > > 2.23.0
> > >

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

[Ard Biesheuvel]

On Wed, 4 Dec 2019 at 15:27, Arvind Sankar <nivedita@alum.mit.edu> 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@alum.mit.edu> 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@alum.mit.edu>
> >
> > 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
> > >

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

[Arvind Sankar]

On Wed, Dec 04, 2019 at 03:30:22PM +0000, Ard Biesheuvel wrote:
> On Wed, 4 Dec 2019 at 15:27, Arvind Sankar <nivedita@alum.mit.edu> 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@alum.mit.edu> 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@alum.mit.edu>
> > >
> > > 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.
> 

Yes, I only have 64-bit x86 hardware.

Re: [PATCH 1/2] efi/gop: Fix return value of setup_gop32/64

[Arvind Sankar]

On Wed, Dec 04, 2019 at 03:28:30PM +0000, Ard Biesheuvel wrote:
> On Wed, 4 Dec 2019 at 15:23, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Wed, Dec 04, 2019 at 03:03:04PM +0000, Ard Biesheuvel wrote:
> > > On Tue, 3 Dec 2019 at 21:47, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > >
> > > > There are two ways the status return of setup_gop32/64 could be
> > > > incorrect.
> > > >
> > > > 1. If we don't find a usable GOP because none of them have a framebuffer
> > > > (i.e. they were all PIXEL_BLT_ONLY), but all the efi calls succeeded, we
> > > > will return EFI_SUCCESS even though we didn't find a usable GOP. Return
> > > > EFI_NOT_FOUND instead, allowing the caller to probe for UGA instead.
> > > >
> > > > 2. If we do find a usable GOP, but one of the subsequent ones fails in
> > > > an EFI call while checking if any support console output, we may return
> > > > an EFI error code even though we found a usable GOP. Fix this by always
> > > > returning EFI_SUCCESS if we got a usable GOP.
> > > >
> > >
> > > Please split this into 2 patches
> >
> > Not quite following -- what should be the two pieces? Are you saying to
> > split the success return and the failure return into two patches -- that
> > seems excessively fine-grained.
> >
> 
> Your commit log describes two distinct changes 1. and 2. The patch
> addresses those two issues, and in addition, moves variable
> declarations around. The result is much more difficult to read than
> necessary.
> 
Ok fair enough. I'll do that in v2 and just leave out the declaration
move then.

[PATCH v2 0/3] Fix a couple of bugs in efi/gop.c

[Arvind Sankar]

Changes from v1:
- Split return value patch into two as requested.
- Remove the __gop_query functions

Arvind Sankar (3):
  efi/gop: Return EFI_NOT_FOUND if there are no usable GOP's
  efi/gop: Return EFI_SUCCESS if a usable GOP was found
  efi/gop: Fix memory leak from __gop_query32/64

 drivers/firmware/efi/libstub/gop.c | 76 +++++++-----------------------
 1 file changed, 17 insertions(+), 59 deletions(-)

-- 
2.23.0

[PATCH v2 1/3] efi/gop: Return EFI_NOT_FOUND if there are no usable GOP's

[Arvind Sankar]

If we don't find a usable GOP because none of them have a framebuffer
(i.e. they were all PIXEL_BLT_ONLY), but all the efi calls succeeded, we
will return EFI_SUCCESS even though we didn't find a usable GOP.

Fix this by explicitly returning EFI_NOT_FOUND if no usable GOP's are
found, allowing the caller to probe for UGA instead.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 drivers/firmware/efi/libstub/gop.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c
index 0101ca4c13b1..62e8b7cff72f 100644
--- a/drivers/firmware/efi/libstub/gop.c
+++ b/drivers/firmware/efi/libstub/gop.c
@@ -175,7 +175,7 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
 
 	/* Did we find any GOPs? */
 	if (!first_gop)
-		goto out;
+		return EFI_NOT_FOUND;
 
 	/* EFI framebuffer */
 	si->orig_video_isVGA = VIDEO_TYPE_EFI;
@@ -197,7 +197,7 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
 	si->lfb_size = si->lfb_linelength * si->lfb_height;
 
 	si->capabilities |= VIDEO_CAPABILITY_SKIP_QUIRKS;
-out:
+
 	return status;
 }
 
@@ -293,7 +293,7 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
 
 	/* Did we find any GOPs? */
 	if (!first_gop)
-		goto out;
+		return EFI_NOT_FOUND;
 
 	/* EFI framebuffer */
 	si->orig_video_isVGA = VIDEO_TYPE_EFI;
@@ -315,7 +315,7 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
 	si->lfb_size = si->lfb_linelength * si->lfb_height;
 
 	si->capabilities |= VIDEO_CAPABILITY_SKIP_QUIRKS;
-out:
+
 	return status;
 }
 
-- 
2.23.0

[PATCH v2 2/3] efi/gop: Return EFI_SUCCESS if a usable GOP was found

[Arvind Sankar]

If we've found a usable GOP with a framebuffer, it is possible that one
of the later EFI calls fails while checking if any support console
output. In this case status may be an EFI error code even though we
found a usable GOP.

Fix this by explicitly return EFI_SUCCESS if a usable GOP has been
located.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 drivers/firmware/efi/libstub/gop.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c
index 62e8b7cff72f..245a5fbef3dc 100644
--- a/drivers/firmware/efi/libstub/gop.c
+++ b/drivers/firmware/efi/libstub/gop.c
@@ -198,7 +198,7 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
 
 	si->capabilities |= VIDEO_CAPABILITY_SKIP_QUIRKS;
 
-	return status;
+	return EFI_SUCCESS;
 }
 
 static efi_status_t
@@ -316,7 +316,7 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
 
 	si->capabilities |= VIDEO_CAPABILITY_SKIP_QUIRKS;
 
-	return status;
+	return EFI_SUCCESS;
 }
 
 /*
-- 
2.23.0

[PATCH v2 3/3] efi/gop: Fix memory leak from __gop_query32/64

[Arvind Sankar]

gop->query_mode returns info in callee-allocated memory which must be
freed by the caller, which we aren't doing.

We don't actually need to call query_mode in order to obtain the info
for the current graphics mode, which is already there in
gop->mode->info, so just access it directly in the setup_gop32/64
functions.

Also nothing uses the size of the info structure, so don't update the
passed-in size (which is the size of the gop_handle table in bytes)
unnecessarily.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 drivers/firmware/efi/libstub/gop.c | 66 ++++++------------------------
 1 file changed, 12 insertions(+), 54 deletions(-)

diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c
index 245a5fbef3dc..7693ec9bbec7 100644
--- a/drivers/firmware/efi/libstub/gop.c
+++ b/drivers/firmware/efi/libstub/gop.c
@@ -83,30 +83,6 @@ setup_pixel_info(struct screen_info *si, u32 pixels_per_scan_line,
 	}
 }
 
-static efi_status_t
-__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)
-{
-	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;
-
-	*fb_base = mode->frame_buffer_base;
-	return status;
-}
-
 static efi_status_t
 setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
             efi_guid_t *proto, unsigned long size, void **gop_handle)
@@ -128,6 +104,7 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
 
 	nr_gops = size / sizeof(u32);
 	for (i = 0; i < nr_gops; i++) {
+		struct efi_graphics_output_protocol_mode_32 *mode;
 		struct efi_graphics_output_mode_info *info = NULL;
 		efi_guid_t conout_proto = EFI_CONSOLE_OUT_DEVICE_GUID;
 		bool conout_found = false;
@@ -145,9 +122,11 @@ 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) &&
+		mode = (void *)(unsigned long)gop32->mode;
+		info = (void *)(unsigned long)mode->info;
+		current_fb_base = mode->frame_buffer_base;
+
+		if ((!first_gop || conout_found) &&
 		    info->pixel_format != PIXEL_BLT_ONLY) {
 			/*
 			 * Systems that use the UEFI Console Splitter may
@@ -201,30 +180,6 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
 	return EFI_SUCCESS;
 }
 
-static efi_status_t
-__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)
-{
-	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;
-
-	*fb_base = mode->frame_buffer_base;
-	return status;
-}
-
 static efi_status_t
 setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
 	    efi_guid_t *proto, unsigned long size, void **gop_handle)
@@ -246,6 +201,7 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
 
 	nr_gops = size / sizeof(u64);
 	for (i = 0; i < nr_gops; i++) {
+		struct efi_graphics_output_protocol_mode_64 *mode;
 		struct efi_graphics_output_mode_info *info = NULL;
 		efi_guid_t conout_proto = EFI_CONSOLE_OUT_DEVICE_GUID;
 		bool conout_found = false;
@@ -263,9 +219,11 @@ 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) &&
+		mode = (void *)(unsigned long)gop64->mode;
+		info = (void *)(unsigned long)mode->info;
+		current_fb_base = mode->frame_buffer_base;
+
+		if ((!first_gop || conout_found) &&
 		    info->pixel_format != PIXEL_BLT_ONLY) {
 			/*
 			 * Systems that use the UEFI Console Splitter may
-- 
2.23.0

Re: [PATCH v2 0/3] Fix a couple of bugs in efi/gop.c

[Ard Biesheuvel]

On Wed, 4 Dec 2019 at 18:17, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> Changes from v1:
> - Split return value patch into two as requested.
> - Remove the __gop_query functions
>
> Arvind Sankar (3):
>   efi/gop: Return EFI_NOT_FOUND if there are no usable GOP's
>   efi/gop: Return EFI_SUCCESS if a usable GOP was found
>   efi/gop: Fix memory leak from __gop_query32/64
>

Thanks Arvind

I queued your changes as fixes here:
https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/log/?h=urgent

I intend to send them out for inclusion in v5.5 in a week or so.


>  drivers/firmware/efi/libstub/gop.c | 76 +++++++-----------------------
>  1 file changed, 17 insertions(+), 59 deletions(-)
>
> --
> 2.23.0
>