* [PATCH v2 1/4] fbdev: Prevent possible use-after-free in fb_release()
2022-05-05 11:31 [PATCH v2 0/4] fbdev: Fix use-after-free caused by wrong fb_info cleanup in drivers Javier Martinez Canillas
@ 2022-05-05 11:31 ` Javier Martinez Canillas
2022-05-05 11:31 ` [PATCH v2 2/4] fbdev: simplefb: Cleanup fb_info in .fb_destroy rather than .remove Javier Martinez Canillas
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Javier Martinez Canillas @ 2022-05-05 11:31 UTC (permalink / raw)
To: linux-kernel
Cc: Daniel Vetter, Daniel Vetter, Javier Martinez Canillas,
Thomas Zimmermann, Daniel Vetter, Helge Deller, dri-devel,
linux-fbdev
From: Daniel Vetter <daniel.vetter@ffwll.ch>
Most fbdev drivers have issues with the fb_info lifetime, because call to
framebuffer_release() from their driver's .remove callback, rather than
doing from fbops.fb_destroy callback.
Doing that will destroy the fb_info too early, while references to it may
still exist, leading to a use-after-free error.
To prevent this, check the fb_info reference counter when attempting to
kfree the data structure in framebuffer_release(). That will leak it but
at least will prevent the mentioned error.
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
---
(no changes since v1)
drivers/video/fbdev/core/fbsysfs.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
index 8c1ee9ecec3d..c2a60b187467 100644
--- a/drivers/video/fbdev/core/fbsysfs.c
+++ b/drivers/video/fbdev/core/fbsysfs.c
@@ -80,6 +80,10 @@ void framebuffer_release(struct fb_info *info)
{
if (!info)
return;
+
+ if (WARN_ON(refcount_read(&info->count)))
+ return;
+
kfree(info->apertures);
kfree(info);
}
--
2.35.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/4] fbdev: simplefb: Cleanup fb_info in .fb_destroy rather than .remove
2022-05-05 11:31 [PATCH v2 0/4] fbdev: Fix use-after-free caused by wrong fb_info cleanup in drivers Javier Martinez Canillas
2022-05-05 11:31 ` [PATCH v2 1/4] fbdev: Prevent possible use-after-free in fb_release() Javier Martinez Canillas
@ 2022-05-05 11:31 ` Javier Martinez Canillas
2022-05-05 11:31 ` [PATCH v2 3/4] fbdev: efifb: " Javier Martinez Canillas
2022-05-05 11:31 ` [PATCH v2 4/4] fbdev: vesafb: " Javier Martinez Canillas
3 siblings, 0 replies; 8+ messages in thread
From: Javier Martinez Canillas @ 2022-05-05 11:31 UTC (permalink / raw)
To: linux-kernel
Cc: Javier Martinez Canillas, Daniel Vetter, Thomas Zimmermann,
Hans de Goede, Helge Deller, dri-devel, linux-fbdev
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 <daniel.vetter@ffwll.ch>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
---
(no changes since v1)
drivers/video/fbdev/simplefb.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 94fc9c6d0411..2c198561c338 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -84,6 +84,10 @@ struct simplefb_par {
static void simplefb_clocks_destroy(struct simplefb_par *par);
static void simplefb_regulators_destroy(struct simplefb_par *par);
+/*
+ * 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 simplefb_destroy(struct fb_info *info)
{
struct simplefb_par *par = info->par;
@@ -94,6 +98,8 @@ static void simplefb_destroy(struct fb_info *info)
if (info->screen_base)
iounmap(info->screen_base);
+ framebuffer_release(info);
+
if (mem)
release_mem_region(mem->start, resource_size(mem));
}
@@ -545,8 +551,8 @@ static int simplefb_remove(struct platform_device *pdev)
{
struct fb_info *info = platform_get_drvdata(pdev);
+ /* simplefb_destroy takes care of info cleanup */
unregister_framebuffer(info);
- framebuffer_release(info);
return 0;
}
--
2.35.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 3/4] fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove
2022-05-05 11:31 [PATCH v2 0/4] fbdev: Fix use-after-free caused by wrong fb_info cleanup in drivers Javier Martinez Canillas
2022-05-05 11:31 ` [PATCH v2 1/4] fbdev: Prevent possible use-after-free in fb_release() Javier Martinez Canillas
2022-05-05 11:31 ` [PATCH v2 2/4] fbdev: simplefb: Cleanup fb_info in .fb_destroy rather than .remove Javier Martinez Canillas
@ 2022-05-05 11:31 ` Javier Martinez Canillas
2022-05-05 11:31 ` [PATCH v2 4/4] fbdev: vesafb: " Javier Martinez Canillas
3 siblings, 0 replies; 8+ messages in thread
From: Javier Martinez Canillas @ 2022-05-05 11:31 UTC (permalink / raw)
To: linux-kernel
Cc: Javier Martinez Canillas, Daniel Vetter, Thomas Zimmermann,
Helge Deller, Peter Jones, dri-devel, linux-fbdev
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 <daniel.vetter@ffwll.ch>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
---
(no changes since v1)
drivers/video/fbdev/efifb.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index ea42ba6445b2..cfa3dc0b4eee 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -243,6 +243,10 @@ static void efifb_show_boot_graphics(struct fb_info *info)
static inline void efifb_show_boot_graphics(struct fb_info *info) {}
#endif
+/*
+ * 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 efifb_destroy(struct fb_info *info)
{
if (efifb_pci_dev)
@@ -254,6 +258,9 @@ static void efifb_destroy(struct fb_info *info)
else
memunmap(info->screen_base);
}
+
+ framebuffer_release(info);
+
if (request_mem_succeeded)
release_mem_region(info->apertures->ranges[0].base,
info->apertures->ranges[0].size);
@@ -620,9 +627,9 @@ static int efifb_remove(struct platform_device *pdev)
{
struct fb_info *info = platform_get_drvdata(pdev);
+ /* efifb_destroy takes care of info cleanup */
unregister_framebuffer(info);
sysfs_remove_groups(&pdev->dev.kobj, efifb_groups);
- framebuffer_release(info);
return 0;
}
--
2.35.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 4/4] fbdev: vesafb: Cleanup fb_info in .fb_destroy rather than .remove
2022-05-05 11:31 [PATCH v2 0/4] fbdev: Fix use-after-free caused by wrong fb_info cleanup in drivers Javier Martinez Canillas
` (2 preceding siblings ...)
2022-05-05 11:31 ` [PATCH v2 3/4] fbdev: efifb: " Javier Martinez Canillas
@ 2022-05-05 11:31 ` Javier Martinez Canillas
2022-05-05 11:51 ` Thomas Zimmermann
2022-05-05 13:02 ` Daniel Vetter
3 siblings, 2 replies; 8+ messages in thread
From: Javier Martinez Canillas @ 2022-05-05 11:31 UTC (permalink / raw)
To: linux-kernel
Cc: Javier Martinez Canillas, Daniel Vetter, Helge Deller, dri-devel,
linux-fbdev
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 <daniel.vetter@ffwll.ch>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 4/4] fbdev: vesafb: Cleanup fb_info in .fb_destroy rather than .remove
2022-05-05 11:31 ` [PATCH v2 4/4] fbdev: vesafb: " Javier Martinez Canillas
@ 2022-05-05 11:51 ` Thomas Zimmermann
2022-05-05 13:02 ` Daniel Vetter
1 sibling, 0 replies; 8+ messages in thread
From: Thomas Zimmermann @ 2022-05-05 11:51 UTC (permalink / raw)
To: Javier Martinez Canillas, linux-kernel
Cc: Daniel Vetter, Helge Deller, dri-devel, linux-fbdev
[-- Attachment #1.1: Type: text/plain, Size: 2598 bytes --]
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 <daniel.vetter@ffwll.ch>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>
> 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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 4/4] fbdev: vesafb: Cleanup fb_info in .fb_destroy rather than .remove
2022-05-05 11:31 ` [PATCH v2 4/4] fbdev: vesafb: " Javier Martinez Canillas
2022-05-05 11:51 ` Thomas Zimmermann
@ 2022-05-05 13:02 ` Daniel Vetter
2022-05-05 13:19 ` Javier Martinez Canillas
1 sibling, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2022-05-05 13:02 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: linux-kernel, Daniel Vetter, Helge Deller, dri-devel, linux-fbdev
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 <daniel.vetter@ffwll.ch>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
> 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 <daniel.vetter@ffwll.ch>
> +
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 4/4] fbdev: vesafb: Cleanup fb_info in .fb_destroy rather than .remove
2022-05-05 13:02 ` Daniel Vetter
@ 2022-05-05 13:19 ` Javier Martinez Canillas
0 siblings, 0 replies; 8+ messages in thread
From: Javier Martinez Canillas @ 2022-05-05 13:19 UTC (permalink / raw)
To: linux-kernel, Helge Deller, dri-devel, linux-fbdev
Hello Daniel,
On 5/5/22 15:02, Daniel Vetter wrote:
[snip]
>> 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")"
>
I think that will just drop this change. While being here I wanted the release
order to be the inverse of the order in which the driver acquires them. But I
will only move the framebuffer_release() that is the problematic bit.
Someone if care enough could fix the rest of the driver.
> Also same comments as on v1 about adding more details about what/how this
> fixes, with that: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
Yes, I'll do that too. Thanks again for your comments and feedback.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
^ permalink raw reply [flat|nested] 8+ messages in thread