2022-05-02 23:47:33

by Javier Martinez Canillas

[permalink] [raw]
Subject: [PATCH v2] fbdev: Make fb_release() return -ENODEV if fbdev was unregistered

A reference to the framebuffer device struct fb_info is stored in the file
private data, but this reference could no longer be valid and must not be
accessed directly. Instead, the file_fb_info() accessor function must be
used since it does sanity checking to make sure that the fb_info is valid.

This can happen for example if the registered framebuffer device is for a
driver that just uses a framebuffer provided by the system firmware. In
that case, the fbdev core would unregister the framebuffer device when a
real video driver is probed and ask to remove conflicting framebuffers.

The bug has been present for a long time but commit 27599aacbaef ("fbdev:
Hot-unplug firmware fb devices on forced removal") unmasked it since the
fbdev core started unregistering the framebuffers' devices associated.

Fixes: 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal")
Reported-by: Maxime Ripard <[email protected]>
Reported-by: Junxiao Chang <[email protected]>
Signed-off-by: Javier Martinez Canillas <[email protected]>
Reviewed-by: Thomas Zimmermann <[email protected]>
---

Changes in v2:
- Drop patch 1/2 since patch 2/2 should be enough to fix the issue.
- Add missing Fixes and Reported-by tags (Thomas Zimmermann).
- Add Thomas Zimmermann's Reviewed-by tag.

drivers/video/fbdev/core/fbmem.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 84427470367b..82d4318ba8f7 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1434,7 +1434,10 @@ fb_release(struct inode *inode, struct file *file)
__acquires(&info->lock)
__releases(&info->lock)
{
- struct fb_info * const info = file->private_data;
+ struct fb_info * const info = file_fb_info(file);
+
+ if (!info)
+ return -ENODEV;

lock_fb_info(info);
if (info->fbops->fb_release)
--
2.35.1


2022-05-03 17:59:51

by Javier Martinez Canillas

[permalink] [raw]
Subject: Re: [PATCH v2] fbdev: Make fb_release() return -ENODEV if fbdev was unregistered

On 5/2/22 15:50, Javier Martinez Canillas wrote:
> A reference to the framebuffer device struct fb_info is stored in the file
> private data, but this reference could no longer be valid and must not be
> accessed directly. Instead, the file_fb_info() accessor function must be
> used since it does sanity checking to make sure that the fb_info is valid.
>
> This can happen for example if the registered framebuffer device is for a
> driver that just uses a framebuffer provided by the system firmware. In
> that case, the fbdev core would unregister the framebuffer device when a
> real video driver is probed and ask to remove conflicting framebuffers.
>
> The bug has been present for a long time but commit 27599aacbaef ("fbdev:
> Hot-unplug firmware fb devices on forced removal") unmasked it since the
> fbdev core started unregistering the framebuffers' devices associated.
>
> Fixes: 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal")
> Reported-by: Maxime Ripard <[email protected]>
> Reported-by: Junxiao Chang <[email protected]>
> Signed-off-by: Javier Martinez Canillas <[email protected]>
> Reviewed-by: Thomas Zimmermann <[email protected]>
> ---
Applied to drm-misc (drm-misc-fixes).

--
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

2022-05-04 14:04:40

by Daniel Vetter

[permalink] [raw]
Subject: Re: [PATCH v2] fbdev: Make fb_release() return -ENODEV if fbdev was unregistered

On Tue, May 03, 2022 at 05:28:09PM +0200, Javier Martinez Canillas wrote:
> On 5/2/22 15:50, Javier Martinez Canillas wrote:
> > A reference to the framebuffer device struct fb_info is stored in the file
> > private data, but this reference could no longer be valid and must not be
> > accessed directly. Instead, the file_fb_info() accessor function must be
> > used since it does sanity checking to make sure that the fb_info is valid.
> >
> > This can happen for example if the registered framebuffer device is for a
> > driver that just uses a framebuffer provided by the system firmware. In
> > that case, the fbdev core would unregister the framebuffer device when a
> > real video driver is probed and ask to remove conflicting framebuffers.
> >
> > The bug has been present for a long time but commit 27599aacbaef ("fbdev:
> > Hot-unplug firmware fb devices on forced removal") unmasked it since the
> > fbdev core started unregistering the framebuffers' devices associated.
> >
> > Fixes: 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal")
> > Reported-by: Maxime Ripard <[email protected]>
> > Reported-by: Junxiao Chang <[email protected]>
> > Signed-off-by: Javier Martinez Canillas <[email protected]>
> > Reviewed-by: Thomas Zimmermann <[email protected]>
> > ---
> Applied to drm-misc (drm-misc-fixes).

See my other reply, but how does this not result in leaks?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch