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 fbdev driver was one that is using a
framebuffer provided by the system firmware. In that case, the fbdev core
could unregister the framebuffer device if a real video driver is probed.
Reported-by: Maxime Ripard <[email protected]>
Signed-off-by: Javier Martinez Canillas <[email protected]>
---
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 20d8929df79f..d68097105f93 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1439,7 +1439,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
Am 02.05.22 um 15:09 schrieb Javier Martinez Canillas:
> 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 fbdev driver was one that is using a
> framebuffer provided by the system firmware. In that case, the fbdev core
> could unregister the framebuffer device if a real video driver is probed.
>
> Reported-by: Maxime Ripard <[email protected]>
> Signed-off-by: Javier Martinez Canillas <[email protected]>
Reviewed-by: Thomas Zimmermann <[email protected]>
This seems like the correct thing to do in any case. Thanks for the
patch. Before merging, you should also add
Fixes: 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced
removal")
Reported-by: Junxiao Chang <[email protected]>
Best regards
Thomas
> ---
>
> 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 20d8929df79f..d68097105f93 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1439,7 +1439,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)
--
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 Mon, May 02, 2022 at 03:09:44PM +0200, 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 fbdev driver was one that is using a
> framebuffer provided by the system firmware. In that case, the fbdev core
> could unregister the framebuffer device if a real video driver is probed.
>
> Reported-by: Maxime Ripard <[email protected]>
> Signed-off-by: Javier Martinez Canillas <[email protected]>
Doesn't this mean we just leak the references? Also anything the driver
might refcount in fb_open would be leaked too.
I'm not sure what exactly you're trying to fix here, but this looks a bit
wrong.
Maybe stepping back what fbdev would need, but doesn't have (see the
commit reference I dropped on the previous version) is drm_dev_enter/exit
around hw access. the file_fb_info check essentially provides that, but
with races and everything.
But drm_dev_enter/exit should not disable sw side code, especially not
refcount cleanup like fb_release does here.
-Daniel
> ---
>
> 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 20d8929df79f..d68097105f93 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1439,7 +1439,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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hello Daniel,
On 5/4/22 11:47, Daniel Vetter wrote:
> On Mon, May 02, 2022 at 03:09:44PM +0200, 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 fbdev driver was one that is using a
>> framebuffer provided by the system firmware. In that case, the fbdev core
>> could unregister the framebuffer device if a real video driver is probed.
>>
>> Reported-by: Maxime Ripard <[email protected]>
>> Signed-off-by: Javier Martinez Canillas <[email protected]>
>
> Doesn't this mean we just leak the references? Also anything the driver
> might refcount in fb_open would be leaked too.
>
It maybe result in leaks, that's true. But I don't think we can do anything
at this point since the fb_info just went away and the file->private_data
reference is no longer valid...
> I'm not sure what exactly you're trying to fix here, but this looks a bit
> wrong.
>
This is fixing a NULL pointer deref that at least 3 people reported, i.e:
https://github.com/raspberrypi/linux/issues/5011
Because if a real DRM driver is probed, then the registered framebuffer
is unregistered and the fb_info just freed. But user-space has no way to
know and on close the kernel will try to dereference a NULL pointer.
> Maybe stepping back what fbdev would need, but doesn't have (see the
> commit reference I dropped on the previous version) is drm_dev_enter/exit
> around hw access. the file_fb_info check essentially provides that, but
> with races and everything.
>
Yes, but I don't know how that could work since user-space can just open
the fbdev, mmap it, write to the mmap'ed memory and then close it. The
only way that this could be done safely AFAICT is if we prevent the real
video drivers to be registered if the fbdev is currently mmap'ed.
Otherwise, the firmware initialized framebuffer will go away anyways and
things will break for the user-space process that's currently using it.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
On Wed, May 04, 2022 at 12:09:57PM +0200, Javier Martinez Canillas wrote:
> Hello Daniel,
>
> On 5/4/22 11:47, Daniel Vetter wrote:
> > On Mon, May 02, 2022 at 03:09:44PM +0200, 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 fbdev driver was one that is using a
> >> framebuffer provided by the system firmware. In that case, the fbdev core
> >> could unregister the framebuffer device if a real video driver is probed.
> >>
> >> Reported-by: Maxime Ripard <[email protected]>
> >> Signed-off-by: Javier Martinez Canillas <[email protected]>
> >
> > Doesn't this mean we just leak the references? Also anything the driver
> > might refcount in fb_open would be leaked too.
> >
>
> It maybe result in leaks, that's true. But I don't think we can do anything
> at this point since the fb_info just went away and the file->private_data
> reference is no longer valid...
>
> > I'm not sure what exactly you're trying to fix here, but this looks a bit
> > wrong.
> >
>
> This is fixing a NULL pointer deref that at least 3 people reported, i.e:
>
> https://github.com/raspberrypi/linux/issues/5011
>
> Because if a real DRM driver is probed, then the registered framebuffer
> is unregistered and the fb_info just freed. But user-space has no way to
> know and on close the kernel will try to dereference a NULL pointer.
The fb_info shouldn't go boom because that's refcounted with
get/put_fb_info. Maybe we go boom on something else, but the fb_info
itself should work out ok. If it doesn't work, then there's a bug and
papering over it by just leaking it all isn't a solution.
> > Maybe stepping back what fbdev would need, but doesn't have (see the
> > commit reference I dropped on the previous version) is drm_dev_enter/exit
> > around hw access. the file_fb_info check essentially provides that, but
> > with races and everything.
> >
>
> Yes, but I don't know how that could work since user-space can just open
> the fbdev, mmap it, write to the mmap'ed memory and then close it. The
> only way that this could be done safely AFAICT is if we prevent the real
> video drivers to be registered if the fbdev is currently mmap'ed.
>
> Otherwise, the firmware initialized framebuffer will go away anyways and
> things will break for the user-space process that's currently using it.
We should probably nuke the mmap and make it SIGBUS. This is essentially
the hotunplug problem, and fixing it properly is very nasty.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch