* [PATCH] fbdev: efifb: Fix a use-after-free due early fb_info cleanup
@ 2022-05-06 13:22 Javier Martinez Canillas
2022-05-06 13:39 ` Thomas Zimmermann
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Javier Martinez Canillas @ 2022-05-06 13:22 UTC (permalink / raw)
To: linux-kernel
Cc: Andrzej Hajda, Ville Syrjälä,
intel-gfx, Javier Martinez Canillas, Daniel Vetter, Helge Deller,
Peter Jones, Thomas Zimmermann, dri-devel, linux-fbdev
Commit d258d00fb9c7 ("fbdev: efifb: Cleanup fb_info in .fb_destroy rather
than .remove") attempted to fix a use-after-free error due driver freeing
the fb_info in the .remove handler instead of doing it in .fb_destroy.
But ironically that change introduced yet another use-after-free since the
fb_info was still used after the free.
This should fix for good by freeing the fb_info at the end of the handler.
Fixes: d258d00fb9c7 ("fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove")
Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reported-by: Andrzej Hajda <andrzej.hajda@intel.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
drivers/video/fbdev/efifb.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index cfa3dc0b4eee..b3d5f884c544 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -259,12 +259,12 @@ static void efifb_destroy(struct fb_info *info)
memunmap(info->screen_base);
}
- framebuffer_release(info);
-
if (request_mem_succeeded)
release_mem_region(info->apertures->ranges[0].base,
info->apertures->ranges[0].size);
fb_dealloc_cmap(&info->cmap);
+
+ framebuffer_release(info);
}
static const struct fb_ops efifb_ops = {
--
2.35.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] fbdev: efifb: Fix a use-after-free due early fb_info cleanup
2022-05-06 13:22 [PATCH] fbdev: efifb: Fix a use-after-free due early fb_info cleanup Javier Martinez Canillas
@ 2022-05-06 13:39 ` Thomas Zimmermann
2022-05-06 15:44 ` [Intel-gfx] " Andi Shyti
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Thomas Zimmermann @ 2022-05-06 13:39 UTC (permalink / raw)
To: Javier Martinez Canillas, linux-kernel
Cc: Andrzej Hajda, Ville Syrjälä,
intel-gfx, Daniel Vetter, Helge Deller, Peter Jones, dri-devel,
linux-fbdev
[-- Attachment #1.1: Type: text/plain, Size: 1751 bytes --]
Am 06.05.22 um 15:22 schrieb Javier Martinez Canillas:
> Commit d258d00fb9c7 ("fbdev: efifb: Cleanup fb_info in .fb_destroy rather
> than .remove") attempted to fix a use-after-free error due driver freeing
> the fb_info in the .remove handler instead of doing it in .fb_destroy.
>
> But ironically that change introduced yet another use-after-free since the
> fb_info was still used after the free.
>
> This should fix for good by freeing the fb_info at the end of the handler.
>
> Fixes: d258d00fb9c7 ("fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove")
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reported-by: Andrzej Hajda <andrzej.hajda@intel.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Thomas Zimmermann <tzimemrmann@suse.de>
> ---
>
> drivers/video/fbdev/efifb.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index cfa3dc0b4eee..b3d5f884c544 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -259,12 +259,12 @@ static void efifb_destroy(struct fb_info *info)
> memunmap(info->screen_base);
> }
>
> - framebuffer_release(info);
> -
> if (request_mem_succeeded)
> release_mem_region(info->apertures->ranges[0].base,
> info->apertures->ranges[0].size);
> fb_dealloc_cmap(&info->cmap);
> +
> + framebuffer_release(info);
> }
>
> static const struct fb_ops efifb_ops = {
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] fbdev: efifb: Fix a use-after-free due early fb_info cleanup
2022-05-06 13:22 [PATCH] fbdev: efifb: Fix a use-after-free due early fb_info cleanup Javier Martinez Canillas
2022-05-06 13:39 ` Thomas Zimmermann
@ 2022-05-06 15:44 ` Andi Shyti
2022-05-06 16:50 ` Andrzej Hajda
2022-05-07 16:20 ` Lucas De Marchi
3 siblings, 0 replies; 6+ messages in thread
From: Andi Shyti @ 2022-05-06 15:44 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: linux-kernel, linux-fbdev, Thomas Zimmermann, Daniel Vetter,
intel-gfx, dri-devel, Peter Jones, Andrzej Hajda, Helge Deller
Hi Javier,
On Fri, May 06, 2022 at 03:22:25PM +0200, Javier Martinez Canillas wrote:
> Commit d258d00fb9c7 ("fbdev: efifb: Cleanup fb_info in .fb_destroy rather
> than .remove") attempted to fix a use-after-free error due driver freeing
> the fb_info in the .remove handler instead of doing it in .fb_destroy.
>
> But ironically that change introduced yet another use-after-free since the
> fb_info was still used after the free.
>
> This should fix for good by freeing the fb_info at the end of the handler.
>
> Fixes: d258d00fb9c7 ("fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove")
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reported-by: Andrzej Hajda <andrzej.hajda@intel.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
Andi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fbdev: efifb: Fix a use-after-free due early fb_info cleanup
2022-05-06 13:22 [PATCH] fbdev: efifb: Fix a use-after-free due early fb_info cleanup Javier Martinez Canillas
2022-05-06 13:39 ` Thomas Zimmermann
2022-05-06 15:44 ` [Intel-gfx] " Andi Shyti
@ 2022-05-06 16:50 ` Andrzej Hajda
2022-05-07 16:20 ` Lucas De Marchi
3 siblings, 0 replies; 6+ messages in thread
From: Andrzej Hajda @ 2022-05-06 16:50 UTC (permalink / raw)
To: Javier Martinez Canillas, linux-kernel
Cc: Ville Syrjälä,
intel-gfx, Daniel Vetter, Helge Deller, Peter Jones,
Thomas Zimmermann, dri-devel, linux-fbdev
On 06.05.2022 15:22, Javier Martinez Canillas wrote:
> Commit d258d00fb9c7 ("fbdev: efifb: Cleanup fb_info in .fb_destroy rather
> than .remove") attempted to fix a use-after-free error due driver freeing
> the fb_info in the .remove handler instead of doing it in .fb_destroy.
>
> But ironically that change introduced yet another use-after-free since the
> fb_info was still used after the free.
>
> This should fix for good by freeing the fb_info at the end of the handler.
>
> Fixes: d258d00fb9c7 ("fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove")
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reported-by: Andrzej Hajda <andrzej.hajda@intel.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
Regards
Andrzej
>
> drivers/video/fbdev/efifb.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index cfa3dc0b4eee..b3d5f884c544 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -259,12 +259,12 @@ static void efifb_destroy(struct fb_info *info)
> memunmap(info->screen_base);
> }
>
> - framebuffer_release(info);
> -
> if (request_mem_succeeded)
> release_mem_region(info->apertures->ranges[0].base,
> info->apertures->ranges[0].size);
> fb_dealloc_cmap(&info->cmap);
> +
> + framebuffer_release(info);
> }
>
> static const struct fb_ops efifb_ops = {
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fbdev: efifb: Fix a use-after-free due early fb_info cleanup
2022-05-06 13:22 [PATCH] fbdev: efifb: Fix a use-after-free due early fb_info cleanup Javier Martinez Canillas
` (2 preceding siblings ...)
2022-05-06 16:50 ` Andrzej Hajda
@ 2022-05-07 16:20 ` Lucas De Marchi
2022-05-07 16:40 ` Javier Martinez Canillas
3 siblings, 1 reply; 6+ messages in thread
From: Lucas De Marchi @ 2022-05-07 16:20 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: linux-kernel, linux-fbdev, Thomas Zimmermann, Daniel Vetter,
intel-gfx, dri-devel, Peter Jones, Andrzej Hajda, Helge Deller
On Fri, May 06, 2022 at 03:22:25PM +0200, Javier Martinez Canillas wrote:
>Commit d258d00fb9c7 ("fbdev: efifb: Cleanup fb_info in .fb_destroy rather
>than .remove") attempted to fix a use-after-free error due driver freeing
>the fb_info in the .remove handler instead of doing it in .fb_destroy.
>
>But ironically that change introduced yet another use-after-free since the
>fb_info was still used after the free.
>
>This should fix for good by freeing the fb_info at the end of the handler.
>
>Fixes: d258d00fb9c7 ("fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove")
are these patches going through any CI before being applied? Maybe would
be a good idea to cc intel-gfx mailing list on these fixes to have Intel
CI to pick them up for some tests?
pushed to drm-misc-fixes where the previous patch was applied.
thanks
LUcas De Marchi
>Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>Reported-by: Andrzej Hajda <andrzej.hajda@intel.com>
>Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>---
>
> drivers/video/fbdev/efifb.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
>index cfa3dc0b4eee..b3d5f884c544 100644
>--- a/drivers/video/fbdev/efifb.c
>+++ b/drivers/video/fbdev/efifb.c
>@@ -259,12 +259,12 @@ static void efifb_destroy(struct fb_info *info)
> memunmap(info->screen_base);
> }
>
>- framebuffer_release(info);
>-
> if (request_mem_succeeded)
> release_mem_region(info->apertures->ranges[0].base,
> info->apertures->ranges[0].size);
> fb_dealloc_cmap(&info->cmap);
>+
>+ framebuffer_release(info);
> }
>
> static const struct fb_ops efifb_ops = {
>--
>2.35.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fbdev: efifb: Fix a use-after-free due early fb_info cleanup
2022-05-07 16:20 ` Lucas De Marchi
@ 2022-05-07 16:40 ` Javier Martinez Canillas
0 siblings, 0 replies; 6+ messages in thread
From: Javier Martinez Canillas @ 2022-05-07 16:40 UTC (permalink / raw)
To: Lucas De Marchi
Cc: linux-fbdev, Andrzej Hajda, Daniel Vetter, intel-gfx,
linux-kernel, dri-devel, Peter Jones, Thomas Zimmermann,
Helge Deller
Hello Lucas,
On 5/7/22 18:20, Lucas De Marchi wrote:
> On Fri, May 06, 2022 at 03:22:25PM +0200, Javier Martinez Canillas wrote:
>> Commit d258d00fb9c7 ("fbdev: efifb: Cleanup fb_info in .fb_destroy rather
>> than .remove") attempted to fix a use-after-free error due driver freeing
>> the fb_info in the .remove handler instead of doing it in .fb_destroy.
>>
>> But ironically that change introduced yet another use-after-free since the
>> fb_info was still used after the free.
>>
>> This should fix for good by freeing the fb_info at the end of the handler.
>>
>> Fixes: d258d00fb9c7 ("fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove")
>
> are these patches going through any CI before being applied? Maybe would
> be a good idea to cc intel-gfx mailing list on these fixes to have Intel
> CI to pick them up for some tests?
>
I Cc'ed intel-gfx for this particular patch. I should had done it for the
previous patches too, but I wasn't aware that Cc'ing that list would make
it run on your CI.
I tested locally the offending patch on an EFI platform before applying it
and I don't know why it didn't fail there. Sorry all for the inconvenience.
> pushed to drm-misc-fixes where the previous patch was applied.
>
Thanks.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-05-07 16:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-06 13:22 [PATCH] fbdev: efifb: Fix a use-after-free due early fb_info cleanup Javier Martinez Canillas
2022-05-06 13:39 ` Thomas Zimmermann
2022-05-06 15:44 ` [Intel-gfx] " Andi Shyti
2022-05-06 16:50 ` Andrzej Hajda
2022-05-07 16:20 ` Lucas De Marchi
2022-05-07 16:40 ` Javier Martinez Canillas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).