Hello,
This series contains patches suggested by Daniel Vetter to fix a use-after-free error in the fb_release() function, due a fb_info associated with a fbdev being freed too early while a user-space process still has the fbdev dev node opened.
That is caused by a wrong management of the struct fb_info lifetime in drivers, but the fbdev core can also be made more resilient about it an leak
This can easily be reproduced with the simplefb driver doing the following:
$ cat < /dev/fb0 & $ echo simple-framebuffer.0 > /sys/bus/platform/drivers/simple-framebuffer/unbind $ kill %1
[ 257.490471] ------------[ cut here ]------------ ... [ 257.495125] refcount_t: underflow; use-after-free. [ 257.495222] WARNING: CPU: 0 PID: 975 at lib/refcount.c:28 refcount_warn_saturate+0xf4/0x144 ... [ 257.637482] pstate: 80400005 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 257.644441] pc : refcount_warn_saturate+0xf4/0x144 [ 257.649226] lr : refcount_warn_saturate+0xf4/0x144 [ 257.654009] sp : ffff80000a06bbf0 [ 257.657315] x29: ffff80000a06bbf0 x28: 000000000000000a x27: 000000000000000a [ 257.664448] x26: 0000000000000000 x25: ffff470b88c6a180 x24: 000000000000000a [ 257.671581] x23: ffff470b81706480 x22: ffff470b808c2160 x21: ffff470b8922ba20 [ 257.678713] x20: ffff470b891f5810 x19: ffff470b891f5800 x18: ffffffffffffffff [ 257.685846] x17: 3a725f7463656a62 x16: ffffbb18c6465fd4 x15: 0720072007200720 [ 257.692978] x14: 0720072d072d072d x13: 0a2e656572662d72 x12: 657466612d657375 [ 257.700110] x11: 203b776f6c667265 x10: 646e75203a745f74 x9 : ffffbb18c58f6c90 [ 257.707242] x8 : 75203b776f6c6672 x7 : 65646e75203a745f x6 : 0000000000000001 [ 257.714373] x5 : ffff470bff8ec418 x4 : 0000000000000000 x3 : 0000000000000027 [ 257.721506] x2 : 0000000000000000 x1 : 0000000000000027 x0 : 0000000000000026 [ 257.728638] Call trace: [ 257.731075] refcount_warn_saturate+0xf4/0x144 [ 257.735513] put_fb_info+0x70/0x7c [ 257.738916] fb_release+0x60/0x74 [ 257.742225] __fput+0x88/0x240 [ 257.745276] ____fput+0x1c/0x30 [ 257.748410] task_work_run+0xc4/0x21c [ 257.752066] do_exit+0x170/0x370 [ 257.755288] do_group_exit+0x40/0xb4 [ 257.758858] get_signal+0x8e0/0x90c [ 257.762339] do_signal+0x1a0/0x280 [ 257.765733] do_notify_resume+0xc8/0x390 [ 257.769650] el0_da+0xe8/0xf0 [ 257.772613] el0t_64_sync_handler+0xe8/0x130 [ 257.776877] el0t_64_sync+0x190/0x194 [ 257.780534] ---[ end trace 0000000000000000 ]---
Patch #1 adds a WARN_ON() to framebuffer_release() to prevent the use-after-free to happen.
Patch #2, #3 and #4 fix the simplefb, efifb and vesafb drivers respectively, to free the resources at the correct place.
Changes in v2: - Also do the change for vesafb (Thomas Zimmermann).
Daniel Vetter (1): fbdev: Prevent possible use-after-free in fb_release()
Javier Martinez Canillas (3): fbdev: simplefb: Cleanup fb_info in .fb_destroy rather than .remove fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove fbdev: vesafb: Cleanup fb_info in .fb_destroy rather than .remove
drivers/video/fbdev/core/fbsysfs.c | 4 ++++ drivers/video/fbdev/efifb.c | 9 ++++++++- drivers/video/fbdev/simplefb.c | 8 +++++++- drivers/video/fbdev/vesafb.c | 14 +++++++++++--- 4 files changed, 30 insertions(+), 5 deletions(-)
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); }
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; }
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; }
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; }
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; }
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
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.
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Javier Martinez Canillas -
Thomas Zimmermann