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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 4 May 2022 12:09:57 +0200
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     linux-kernel@...r.kernel.org, Maxime Ripard <maxime@...no.tech>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Alex Deucher <alexander.deucher@....com>,
        Changcheng Deng <deng.changcheng@....com.cn>,
        Guenter Roeck <linux@...ck-us.net>,
        Helge Deller <deller@....de>, Sam Ravnborg <sam@...nborg.org>,
        Zhen Lei <thunder.leizhen@...wei.com>,
        Zheyu Ma <zheyuma97@...il.com>,
        dri-devel@...ts.freedesktop.org, linux-fbdev@...r.kernel.org
Subject: Re: [PATCH 2/2] fbdev: Make fb_release() return -ENODEV if fbdev was
 unregistered

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@...no.tech>
>> Signed-off-by: Javier Martinez Canillas <javierm@...hat.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