| Message ID | 20220505113128.264963-5-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | fbdev: Fix use-after-free caused by wrong fb_info cleanup in drivers | expand |
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.
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; }
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(-)