Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 4 May 2022 12:15:02 +0200
From:   Daniel Vetter <daniel@ffwll.ch>
To:     Javier Martinez Canillas <javierm@redhat.com>
Cc:     linux-kernel@vger.kernel.org, Maxime Ripard <maxime@cerno.tech>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Alex Deucher <alexander.deucher@amd.com>,
        Changcheng Deng <deng.changcheng@zte.com.cn>,
        Guenter Roeck <linux@roeck-us.net>,
        Helge Deller <deller@gmx.de>, Sam Ravnborg <sam@ravnborg.org>,
        Zhen Lei <thunder.leizhen@huawei.com>,
        Zheyu Ma <zheyuma97@gmail.com>,
        dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org
Subject: Re: [PATCH 2/2] fbdev: Make fb_release() return -ENODEV if fbdev was
 unregistered
Message-ID: <YnJSJideWoEF+ZE0@phenom.ffwll.local>
Mail-Followup-To: Javier Martinez Canillas <javierm@redhat.com>,
        linux-kernel@vger.kernel.org, Maxime Ripard <maxime@cerno.tech>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Alex Deucher <alexander.deucher@amd.com>,
        Changcheng Deng <deng.changcheng@zte.com.cn>,
        Guenter Roeck <linux@roeck-us.net>, Helge Deller <deller@gmx.de>,
        Sam Ravnborg <sam@ravnborg.org>,
        Zhen Lei <thunder.leizhen@huawei.com>,
        Zheyu Ma <zheyuma97@gmail.com>, dri-devel@lists.freedesktop.org,
        linux-fbdev@vger.kernel.org
References: <20220502130944.363776-1-javierm@redhat.com>
 <20220502130944.363776-3-javierm@redhat.com>
 <YnJLzY7Yiax/AwMx@phenom.ffwll.local>
 <8a4d6469-d3c0-833d-40c8-3a786d04eba4@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8a4d6469-d3c0-833d-40c8-3a786d04eba4@redhat.com>
Precedence: bulk

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