[Intel-gfx] [PATCH 1/2] drm/core: Preserve the framebuffer after removing it.

[Daniel Vetter]

On Wed, Sep 09, 2015 at 04:47:08PM +0100, Tvrtko Ursulin wrote:
> 
> On 09/09/2015 04:29 PM, Daniel Vetter wrote:
> >On Wed, Sep 09, 2015 at 04:18:02PM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 09/09/2015 04:04 PM, Daniel Vetter wrote:
> >>>On Wed, Sep 09, 2015 at 03:51:50PM +0100, Tvrtko Ursulin wrote:
> >>>>
> >>>>Hi,
> >>>>
> >>>>On 09/09/2015 03:40 PM, Maarten Lankhorst wrote:
> >>>>>Previously RMFB and fd close chose to disable any plane that had
> >>>>>an active framebuffer from this file. If it was a primary plane the
> >>>>>crtc was disabled. However the fbdev code or any system compositor
> >>>>>should restore the planes anyway so there's no need to do it twice.
> >>>>>
> >>>>>The old fb_id is zero'd, so there's no danger of being able to
> >>>>>restore the fb from fb_id.
> >>>>
> >>>>What does this mean, say if the compositor dies last frame will remain on
> >>>>the screen?
> >>>
> >>>Yes, and the commit message should mention that. It should also mention
> >>>that other applications can't get at the data since we clear fb id still,
> >>>so no information leak there.
> >>
> >>Perhaps I replied to the wrong patch from the series.
> >>
> >>Why is all this needed anyway? It sound pretty undesirable from the security
> >>point of view to me. If it is exploitable to leave something sensitive on
> >>screen that's not good.
> >
> >fd close is a super-painful context to do a full-blown modeset. It's
> >userspace but we can't restart anything because no one ever checks the
> >return value of close(). We could fix it by pushing this to a work item,
> >but given that the rule itself seems dubious it's easier to adjust the abi
> >imo. Framebuffers are somewhat global, so not deleting them makes imo
> >sense.
> >
> >The big change is patch 2, which will make them survive for real.
> 
> I don't follow this closely but it still sounds wrong. If modeset is a
> concern then disable the planes and/or clear them?

This is generic core code, you can't disable/clear planes in a generic way
without doing a full modeset.

> It really doesn't feel preservation of fb content is a good thing to do. If
> the higher goal is to enable some smooth transitions clients should engineer
> that themselves.
> 
> In any case leaving content on screen sounds really bad to me.
> 
> Reminds me of screen locker bugs which sometimes did not clear the screen
> when displaying the unlock dialog. That was pretty common for a long period
> in KDE. And this sounds like it could be attackable in a similar way.

Afaik that's just userspace coordination fail - system deamons go into
suspend without telling and waiting for the screenlocker to lock the
screen. Hilarious, but not really something we can fix in the kernel.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch