Messages in this thread | | | Date | Mon, 2 May 2022 15:36:20 +0200 | Subject | Re: [PATCH 1/2] fbdev: Check in file_fb_info() if the fb_info was already been freed | From | Javier Martinez Canillas <> |
| |
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.
-- Best regards,
Javier Martinez Canillas Linux Engineering Red Hat
| |