| Message ID | 20220502130944.363776-3-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | fbdev: Fix a NULL pointer dereference in fb_release() | expand |
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
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)
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(-)