2022-05-09 05:08:30

by Javier Martinez Canillas

Subject: [PATCH] fbdev: efifb: Fix a use-after-free due early fb_info cleanup

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ä <[email protected]>
Reported-by: Andrzej Hajda <[email protected]>
Signed-off-by: Javier Martinez Canillas <[email protected]>
---

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

2022-05-09 08:01:02

by Andi Shyti

Subject: Re: [Intel-gfx] [PATCH] fbdev: efifb: Fix a use-after-free due early fb_info cleanup

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? <[email protected]>
> Reported-by: Andrzej Hajda <[email protected]>
> Signed-off-by: Javier Martinez Canillas <[email protected]>

Reviewed-by: Andi Shyti <[email protected]>

Andi

2022-05-09 11:28:42

by Andrzej Hajda

Subject: Re: [PATCH] fbdev: efifb: Fix a use-after-free due early fb_info cleanup

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ä <[email protected]>
> Reported-by: Andrzej Hajda <[email protected]>
> Signed-off-by: Javier Martinez Canillas <[email protected]>
> ---

Reviewed-by: Andrzej Hajda <[email protected]>

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