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.