[PATCH 0/2] fbdev: Fix a NULL pointer dereference in fb_release()

[PATCH 0/2] fbdev: Fix a NULL pointer dereference in fb_release()

[Javier Martinez Canillas]

Hello,

This small series contains fixes for two bugs I found in fbmem.c, that may
explain a bug reported in the rpi4 [0] when using simplefb and vc4 drivers.

I was not able to reproduce the mentioned bug, but looking at the code I
think that it might explain the issue.

I've tested these patches in a rpi4 with both simplefb and vc4 drivers
built-in and did not find any regression.

[0]: https://github.com/raspberrypi/linux/issues/5011

Best regards,
Javier


Javier Martinez Canillas (2):
  fbdev: Check in file_fb_info() if the fb_info was already been freed
  fbdev: Make fb_release() return -ENODEV if fbdev was unregistered

 drivers/video/fbdev/core/fbmem.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

-- 
2.35.1

[PATCH 1/2] fbdev: Check in file_fb_info() if the fb_info was already been freed

[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;
+
 	return info;
 }
 
-- 
2.35.1

Re: [PATCH 1/2] fbdev: Check in file_fb_info() if the fb_info was already been freed

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2050 bytes --]

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;
>   }
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 1/2] fbdev: Check in file_fb_info() if the fb_info was already been freed

[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

[PATCH 2/2] fbdev: Make fb_release() return -ENODEV if fbdev was unregistered

[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>
---

 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

Re: [PATCH 2/2] fbdev: Make fb_release() return -ENODEV if fbdev was unregistered

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1934 bytes --]



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)

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 2/2] fbdev: Make fb_release() return -ENODEV if fbdev was unregistered

[Javier Martinez Canillas]

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

Re: [PATCH 2/2] fbdev: Make fb_release() return -ENODEV if fbdev was unregistered

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/2] fbdev: Make fb_release() return -ENODEV if fbdev was unregistered

[Javier Martinez Canillas]

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.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 2/2] fbdev: Make fb_release() return -ENODEV if fbdev was unregistered

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch