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