The driver is calling framebuffer_release() in its .remove callback, but
this will cause the struct fb_info to be freed too early. Since it could
be that a reference is still hold to it if user-space opened the fbdev.
This would lead to a use-after-free error if the framebuffer device was
unregistered but later a user-space process tries to close the fbdev fd.
The correct thing to do is to only unregister the framebuffer in the
driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.
Suggested-by: Daniel Vetter <[email protected]>
Signed-off-by: Javier Martinez Canillas <[email protected]>
---
Changes in v2:
- Also do the change for vesafb (Thomas Zimmermann).
drivers/video/fbdev/vesafb.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/video/fbdev/vesafb.c b/drivers/video/fbdev/vesafb.c
index df6de5a9dd4c..1f03a449e505 100644
--- a/drivers/video/fbdev/vesafb.c
+++ b/drivers/video/fbdev/vesafb.c
@@ -179,6 +179,10 @@ static int vesafb_setcolreg(unsigned regno, unsigned red, unsigned green,
return err;
}
+/*
+ * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
+ * of unregister_framebuffer() or fb_release(). Do any cleanup here.
+ */
static void vesafb_destroy(struct fb_info *info)
{
struct vesafb_par *par = info->par;
@@ -187,7 +191,13 @@ static void vesafb_destroy(struct fb_info *info)
arch_phys_wc_del(par->wc_cookie);
if (info->screen_base)
iounmap(info->screen_base);
+
+ if (((struct vesafb_par *)(info->par))->region)
+ release_region(0x3c0, 32);
+
release_mem_region(info->apertures->ranges[0].base, info->apertures->ranges[0].size);
+
+ framebuffer_release(info);
}
static struct fb_ops vesafb_ops = {
@@ -484,10 +494,8 @@ static int vesafb_remove(struct platform_device *pdev)
{
struct fb_info *info = platform_get_drvdata(pdev);
+ /* vesafb_destroy takes care of info cleanup */
unregister_framebuffer(info);
- if (((struct vesafb_par *)(info->par))->region)
- release_region(0x3c0, 32);
- framebuffer_release(info);
return 0;
}
--
2.35.1
Am 05.05.22 um 13:31 schrieb Javier Martinez Canillas:
> The driver is calling framebuffer_release() in its .remove callback, but
> this will cause the struct fb_info to be freed too early. Since it could
> be that a reference is still hold to it if user-space opened the fbdev.
>
> This would lead to a use-after-free error if the framebuffer device was
> unregistered but later a user-space process tries to close the fbdev fd.
>
> The correct thing to do is to only unregister the framebuffer in the
> driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.
>
> Suggested-by: Daniel Vetter <[email protected]>
> Signed-off-by: Javier Martinez Canillas <[email protected]>
Reviewed-by: Thomas Zimmermann <[email protected]>
> ---
>
> Changes in v2:
> - Also do the change for vesafb (Thomas Zimmermann).
>
> drivers/video/fbdev/vesafb.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/fbdev/vesafb.c b/drivers/video/fbdev/vesafb.c
> index df6de5a9dd4c..1f03a449e505 100644
> --- a/drivers/video/fbdev/vesafb.c
> +++ b/drivers/video/fbdev/vesafb.c
> @@ -179,6 +179,10 @@ static int vesafb_setcolreg(unsigned regno, unsigned red, unsigned green,
> return err;
> }
>
> +/*
> + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
> + * of unregister_framebuffer() or fb_release(). Do any cleanup here.
> + */
> static void vesafb_destroy(struct fb_info *info)
> {
> struct vesafb_par *par = info->par;
> @@ -187,7 +191,13 @@ static void vesafb_destroy(struct fb_info *info)
> arch_phys_wc_del(par->wc_cookie);
> if (info->screen_base)
> iounmap(info->screen_base);
> +
> + if (((struct vesafb_par *)(info->par))->region)
> + release_region(0x3c0, 32);
> +
> release_mem_region(info->apertures->ranges[0].base, info->apertures->ranges[0].size);
> +
> + framebuffer_release(info);
> }
>
> static struct fb_ops vesafb_ops = {
> @@ -484,10 +494,8 @@ static int vesafb_remove(struct platform_device *pdev)
> {
> struct fb_info *info = platform_get_drvdata(pdev);
>
> + /* vesafb_destroy takes care of info cleanup */
> unregister_framebuffer(info);
> - if (((struct vesafb_par *)(info->par))->region)
> - release_region(0x3c0, 32);
> - framebuffer_release(info);
>
> return 0;
> }
--
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
On Thu, May 05, 2022 at 01:31:27PM +0200, Javier Martinez Canillas wrote:
> The driver is calling framebuffer_release() in its .remove callback, but
> this will cause the struct fb_info to be freed too early. Since it could
> be that a reference is still hold to it if user-space opened the fbdev.
>
> This would lead to a use-after-free error if the framebuffer device was
> unregistered but later a user-space process tries to close the fbdev fd.
>
> The correct thing to do is to only unregister the framebuffer in the
> driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.
>
> Suggested-by: Daniel Vetter <[email protected]>
> Signed-off-by: Javier Martinez Canillas <[email protected]>
> ---
>
> Changes in v2:
> - Also do the change for vesafb (Thomas Zimmermann).
>
> drivers/video/fbdev/vesafb.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/fbdev/vesafb.c b/drivers/video/fbdev/vesafb.c
> index df6de5a9dd4c..1f03a449e505 100644
> --- a/drivers/video/fbdev/vesafb.c
> +++ b/drivers/video/fbdev/vesafb.c
> @@ -179,6 +179,10 @@ static int vesafb_setcolreg(unsigned regno, unsigned red, unsigned green,
> return err;
> }
>
> +/*
> + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
> + * of unregister_framebuffer() or fb_release(). Do any cleanup here.
> + */
> static void vesafb_destroy(struct fb_info *info)
> {
> struct vesafb_par *par = info->par;
> @@ -187,7 +191,13 @@ static void vesafb_destroy(struct fb_info *info)
> arch_phys_wc_del(par->wc_cookie);
> if (info->screen_base)
> iounmap(info->screen_base);
> +
> + if (((struct vesafb_par *)(info->par))->region)
> + release_region(0x3c0, 32);
This move seems rather iffy, so maybe justify it with "makes the code
exactly as busted before 27599aacbaef ("fbdev: Hot-unplug firmware fb
devices on forced removal")"
Also same comments as on v1 about adding more details about what/how this
fixes, with that: Reviewed-by: Daniel Vetter <[email protected]>
> +
> release_mem_region(info->apertures->ranges[0].base, info->apertures->ranges[0].size);
> +
> + framebuffer_release(info);
> }
>
> static struct fb_ops vesafb_ops = {
> @@ -484,10 +494,8 @@ static int vesafb_remove(struct platform_device *pdev)
> {
> struct fb_info *info = platform_get_drvdata(pdev);
>
> + /* vesafb_destroy takes care of info cleanup */
> unregister_framebuffer(info);
> - if (((struct vesafb_par *)(info->par))->region)
> - release_region(0x3c0, 32);
> - framebuffer_release(info);
>
> return 0;
> }
> --
> 2.35.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch