Hello,
This small series contains fixes for two bugs I found in fbmem.c, that may explain a bug reported in the rpi4 [0] when using simplefb and vc4 drivers.
I was not able to reproduce the mentioned bug, but looking at the code I think that it might explain the issue.
I've tested these patches in a rpi4 with both simplefb and vc4 drivers built-in and did not find any regression.
[0]: https://github.com/raspberrypi/linux/issues/5011
Best regards, Javier
Javier Martinez Canillas (2): fbdev: Check in file_fb_info() if the fb_info was already been freed fbdev: Make fb_release() return -ENODEV if fbdev was unregistered
drivers/video/fbdev/core/fbmem.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
If real driver probes, the fbdev core kicks out all drivers that are using a framebuffer that were provided by the system firmware. But it could be a user-space process still has a file descriptor for the fbdev device node.
This can lead to a NULL pointer dereference, if the framebuffer device is unregistered and associated data freed, but later in the .release callback is attempted to access its struct fb_info.
To prevent this, make file_fb_info() to also check the fb_info reference counter and just return NULL if this equals zero. Since that means it has already been freed.
Signed-off-by: Javier Martinez Canillas javierm@redhat.com ---
drivers/video/fbdev/core/fbmem.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 84427470367b..20d8929df79f 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -751,8 +751,13 @@ static struct fb_info *file_fb_info(struct file *file) int fbidx = iminor(inode); struct fb_info *info = registered_fb[fbidx];
- if (info != file->private_data) - info = NULL; + if (!info) + return NULL; + + /* check that the fb_info has not changed or was already freed */ + if (info != file->private_data || refcount_read(&info->count) == 0) + return NULL; + return info; }
Hi
Am 02.05.22 um 15:09 schrieb Javier Martinez Canillas:
If real driver probes, the fbdev core kicks out all drivers that are using a framebuffer that were provided by the system firmware. But it could be a user-space process still has a file descriptor for the fbdev device node.
This can lead to a NULL pointer dereference, if the framebuffer device is unregistered and associated data freed, but later in the .release callback is attempted to access its struct fb_info.
To prevent this, make file_fb_info() to also check the fb_info reference counter and just return NULL if this equals zero. Since that means it has already been freed.
Signed-off-by: Javier Martinez Canillas javierm@redhat.com
drivers/video/fbdev/core/fbmem.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 84427470367b..20d8929df79f 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -751,8 +751,13 @@ static struct fb_info *file_fb_info(struct file *file) int fbidx = iminor(inode); struct fb_info *info = registered_fb[fbidx];
- if (info != file->private_data)
info = NULL;
- if (!info)
return NULL;- /* check that the fb_info has not changed or was already freed */
- if (info != file->private_data || refcount_read(&info->count) == 0)
return NULL;
Acked-by: Thomas Zimmermann tzimmermann@suse.de
However, I'm having problems with the semantics of these variables: if we have an info from registered_fb[fbinx] and the refcount in info->count is still 0, isn't that a consistency problem? If so, we should print a WARN_ON().
Best regards Thomas
return info; }
Hello Thomas,
On 5/2/22 15:26, Thomas Zimmermann wrote:
Hi
Am 02.05.22 um 15:09 schrieb Javier Martinez Canillas:
If real driver probes, the fbdev core kicks out all drivers that are using a framebuffer that were provided by the system firmware. But it could be a user-space process still has a file descriptor for the fbdev device node.
This can lead to a NULL pointer dereference, if the framebuffer device is unregistered and associated data freed, but later in the .release callback is attempted to access its struct fb_info.
To prevent this, make file_fb_info() to also check the fb_info reference counter and just return NULL if this equals zero. Since that means it has already been freed.
Signed-off-by: Javier Martinez Canillas javierm@redhat.com
drivers/video/fbdev/core/fbmem.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 84427470367b..20d8929df79f 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -751,8 +751,13 @@ static struct fb_info *file_fb_info(struct file *file) int fbidx = iminor(inode); struct fb_info *info = registered_fb[fbidx];
- if (info != file->private_data)
info = NULL;
- if (!info)
return NULL;- /* check that the fb_info has not changed or was already freed */
- if (info != file->private_data || refcount_read(&info->count) == 0)
return NULL;Acked-by: Thomas Zimmermann tzimmermann@suse.de
However, I'm having problems with the semantics of these variables: if we have an info from registered_fb[fbinx] and the refcount in info->count is still 0, isn't that a consistency problem? If so, we should print a WARN_ON().
That's a good point. Maybe we are being too paranoid here? If the fb_info was set to NULL then the existing if (info != file->private_data) check will already catch that issue.
In other words, now that fb_release() is getting the fb_info with the file_fb_info() function instead of file->private_data directly, the NULL pointer dereference should not happen anymore.
I think that will just drop this patch, the less we touch the fbdev code the better IMO.
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 maxime@cerno.tech Signed-off-by: Javier Martinez Canillas javierm@redhat.com ---
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)
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 maxime@cerno.tech Signed-off-by: Javier Martinez Canillas javierm@redhat.com
Reviewed-by: Thomas Zimmermann tzimmermann@suse.de
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 junxiao.chang@intel.com
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)
Hello Thomas,
On 5/2/22 15:20, Thomas Zimmermann wrote:
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 maxime@cerno.tech Signed-off-by: Javier Martinez Canillas javierm@redhat.com
Reviewed-by: Thomas Zimmermann tzimmermann@suse.de
Thanks.
This seems like the correct thing to do in any case. Thanks for the
Agreed, it's certainly a bug if not the same that was already reported.
patch. Before merging, you should also add
Fixes: 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal")
I thought about that but I don't think that's accurate since the bug is not related to that commit. That might make easier to reproduce it but is something that would happen anyway if for example someone attempted to remove a module or unbind the device using the sysfs entries.
Maybe I can comment in the commit message that this change made it more likely to occur and for that reason I'm adding a fixes tag.
Reported-by: Junxiao Chang junxiao.chang@intel.com
Indeed. -- Best regards,
Javier Martinez Canillas Linux Engineering Red Hat
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 maxime@cerno.tech Signed-off-by: Javier Martinez Canillas javierm@redhat.com
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
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 maxime@cerno.tech Signed-off-by: Javier Martinez Canillas javierm@redhat.com
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.
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 maxime@cerno.tech Signed-off-by: Javier Martinez Canillas javierm@redhat.com
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Javier Martinez Canillas -
Thomas Zimmermann