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

From: Daniel Vetter <daniel@ffwll.ch>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 1/2] drm/core: Preserve the framebuffer after removing it.
Date: Wed, 9 Sep 2015 17:56:15 +0200	[thread overview]
Message-ID: <20150909155615.GU2767@phenom.ffwll.local> (raw)
In-Reply-To: <55F0547C.10204@linux.intel.com>

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel