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@xxxxxxxxxx> >> --- >> >> 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@xxxxxxx> > > 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
