[PATCH 0/2] Preserve framebuffer during rmfb / drm fd close.

[PATCH 0/2] Preserve framebuffer during rmfb / drm fd close.

[Maarten Lankhorst]

This is breaks the abi slightly, but allows preserving the framebuffer contents across processes.
Any system compositor or fbdev should take care of resetting the planes and mode anyway.

Restoring a framebuffer requires a call to getfb, which checks for
CAP_SYS_ADMIN, DRM_MASTER or access to the control file. Any of those
require privileges, so this shouldn't be a security issue.

Maarten Lankhorst (2):
  drm/core: Preserve the framebuffer after removing it.
  drm/core: Preserve the fb id on close.

 drivers/gpu/drm/drm_crtc.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Maarten Lankhorst]

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.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 9b9c4b41422a..626b0a57efbf 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3327,7 +3327,7 @@ int drm_mode_rmfb(struct drm_device *dev,
 	mutex_unlock(&dev->mode_config.fb_lock);
 	mutex_unlock(&file_priv->fbs_lock);
 
-	drm_framebuffer_remove(fb);
+	drm_framebuffer_unreference(fb);
 
 	return 0;
 
@@ -3517,7 +3517,7 @@ void drm_fb_release(struct drm_file *priv)
 		list_del_init(&fb->filp_head);
 
 		/* This will also drop the fpriv->fbs reference. */
-		drm_framebuffer_remove(fb);
+		drm_framebuffer_unreference(fb);
 	}
 }
 
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Tvrtko Ursulin]

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?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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

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

[Tvrtko Ursulin]

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.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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

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

[Tvrtko Ursulin]

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?

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.

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Tvrtko Ursulin]

On 09/09/2015 04:56 PM, Daniel Vetter wrote:
> 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.

It was just an example of a class of vulnerabilities which would be 
possible with these changes. If they, as you said, will preserve the 
last frame on screen when the compositor crashes.

For me this is serious enough not to go this route.

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Wed, Sep 9, 2015 at 6:03 PM, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
> It was just an example of a class of vulnerabilities which would be possible
> with these changes. If they, as you said, will preserve the last frame on
> screen when the compositor crashes.

If your compositor crashes something should take over, either fbdev
(which force-restores) or a new compositor (system one or just the one
that crashed, restarted). And on modern userspace logind has copies of
the fds which it uses to make sure priviledges (i.e. master rights)
don't escape to the wrong person.

> For me this is serious enough not to go this route.

If that doesn't happen you have yet another bug in userspace. I don't
think there's a real problem really.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 09/09/2015 05:07 PM, Daniel Vetter wrote:
> On Wed, Sep 9, 2015 at 6:03 PM, Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>> It was just an example of a class of vulnerabilities which would be possible
>> with these changes. If they, as you said, will preserve the last frame on
>> screen when the compositor crashes.
>
> If your compositor crashes something should take over, either fbdev
> (which force-restores) or a new compositor (system one or just the one
> that crashed, restarted). And on modern userspace logind has copies of
> the fds which it uses to make sure priviledges (i.e. master rights)
> don't escape to the wrong person.

The famous "should". fbdev is going out no? And attack just needs to 
prevent compositor from starting again. Or a bug somewhere needs to do 
that. Fact remains, before this = black screen, after this = last frame 
with bank details or similar.

Change makes the scenario more likely, so what is the justification? 
Only that modeset is hard on framebuffer owner exiting?

>> For me this is serious enough not to go this route.
>
> If that doesn't happen you have yet another bug in userspace. I don't
> think there's a real problem really.

If white hats had the imagination of black hats there would be no 
problems whatsoever. :)

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Maarten Lankhorst]

Op 09-09-15 om 18:15 schreef Tvrtko Ursulin:
>
> On 09/09/2015 05:07 PM, Daniel Vetter wrote:
>> On Wed, Sep 9, 2015 at 6:03 PM, Tvrtko Ursulin
>> <tvrtko.ursulin@linux.intel.com> wrote:
>>> It was just an example of a class of vulnerabilities which would be possible
>>> with these changes. If they, as you said, will preserve the last frame on
>>> screen when the compositor crashes.
>>
>> If your compositor crashes something should take over, either fbdev
>> (which force-restores) or a new compositor (system one or just the one
>> that crashed, restarted). And on modern userspace logind has copies of
>> the fds which it uses to make sure priviledges (i.e. master rights)
>> don't escape to the wrong person.
>
> The famous "should". fbdev is going out no? And attack just needs to prevent compositor from starting again. Or a bug somewhere needs to do that. Fact remains, before this = black screen, after this = last frame with bank details or similar.
>
> Change makes the scenario more likely, so what is the justification? Only that modeset is hard on framebuffer owner exiting?
>>> For me this is serious enough not to go this route.
>>
>> If that doesn't happen you have yet another bug in userspace. I don't
>> think there's a real problem really.
>
> If white hats had the imagination of black hats there would be no problems whatsoever. :)
>
> Tvrtko

I have enough imagination, but the fact is the code to copy the fb contents requires the following:

file_priv->is_master || capable(CAP_SYS_ADMIN) || drm_is_control_client(file_priv)

If you already have any of those privileges you can draw your own fake TTY login screen
and grab the password that way, so I don't see an additional attack vector exposed here.

~Maarten

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 09/09/2015 05:26 PM, Maarten Lankhorst wrote:
> Op 09-09-15 om 18:15 schreef Tvrtko Ursulin:
>>
>> On 09/09/2015 05:07 PM, Daniel Vetter wrote:
>>> On Wed, Sep 9, 2015 at 6:03 PM, Tvrtko Ursulin
>>> <tvrtko.ursulin@linux.intel.com> wrote:
>>>> It was just an example of a class of vulnerabilities which would be possible
>>>> with these changes. If they, as you said, will preserve the last frame on
>>>> screen when the compositor crashes.
>>>
>>> If your compositor crashes something should take over, either fbdev
>>> (which force-restores) or a new compositor (system one or just the one
>>> that crashed, restarted). And on modern userspace logind has copies of
>>> the fds which it uses to make sure priviledges (i.e. master rights)
>>> don't escape to the wrong person.
>>
>> The famous "should". fbdev is going out no? And attack just needs to prevent compositor from starting again. Or a bug somewhere needs to do that. Fact remains, before this = black screen, after this = last frame with bank details or similar.
>>
>> Change makes the scenario more likely, so what is the justification? Only that modeset is hard on framebuffer owner exiting?
>>>> For me this is serious enough not to go this route.
>>>
>>> If that doesn't happen you have yet another bug in userspace. I don't
>>> think there's a real problem really.
>>
>> If white hats had the imagination of black hats there would be no problems whatsoever. :)
>>
>> Tvrtko
>
> I have enough imagination, but the fact is the code to copy the fb contents requires the following:
>
> file_priv->is_master || capable(CAP_SYS_ADMIN) || drm_is_control_client(file_priv)
>
> If you already have any of those privileges you can draw your own fake TTY login screen
> and grab the password that way, so I don't see an additional attack vector exposed here.

I am not even going that far, just talking about last frame stuck on 
screen. For me making that easier is a regression.

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Wed, Sep 9, 2015 at 6:36 PM, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
> I am not even going that far, just talking about last frame stuck on screen.
> For me making that easier is a regression.

So let's look at various systems:
- super-modern fbdev less system: logind keeps a dup of every
master-capabel drm fd. Compositor crashing won't ever result in
close() getting called since logind still has its copy. Cleanup needs
to be done manually anyway with the system compositor.
- Current systems: Compositor restarts and cleans up the mess we left behind.
- Greybeards who start X with startx: Those folks also have fbdev,
which will do the recover.

In the strictest sense the screen leaks for a bit. In practice no one
will ever notice, at least assuming I haven't missed a use-case. And
for regression it only counts as one if you can actually spot a
difference ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Tvrtko Ursulin]

On 09/09/2015 08:06 PM, Daniel Vetter wrote:
> On Wed, Sep 9, 2015 at 6:36 PM, Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>> I am not even going that far, just talking about last frame stuck on screen.
>> For me making that easier is a regression.
>
> So let's look at various systems:
> - super-modern fbdev less system: logind keeps a dup of every
> master-capabel drm fd. Compositor crashing won't ever result in
> close() getting called since logind still has its copy. Cleanup needs
> to be done manually anyway with the system compositor.
> - Current systems: Compositor restarts and cleans up the mess we left behind.

What if the compositor doesn't restart? Or logind crashes in the former 
case?

Maybe I don't understand something, but I don't see how it is not quite 
bad to expect userspace to clean up the kernel structures after the 
previous userspace client.

What happens if something keeps crashing leaving framebuffers around?

If the only reason is to avoid modeset, why SETPLANE with NULL fb to 
disable planes associated with a framebuffers to be released wouldn't work?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Thu, Sep 10, 2015 at 10:07:41AM +0100, Tvrtko Ursulin wrote:
> 
> On 09/09/2015 08:06 PM, Daniel Vetter wrote:
> >On Wed, Sep 9, 2015 at 6:36 PM, Tvrtko Ursulin
> ><tvrtko.ursulin@linux.intel.com> wrote:
> >>I am not even going that far, just talking about last frame stuck on screen.
> >>For me making that easier is a regression.
> >
> >So let's look at various systems:
> >- super-modern fbdev less system: logind keeps a dup of every
> >master-capabel drm fd. Compositor crashing won't ever result in
> >close() getting called since logind still has its copy. Cleanup needs
> >to be done manually anyway with the system compositor.
> >- Current systems: Compositor restarts and cleans up the mess we left behind.
> 
> What if the compositor doesn't restart? Or logind crashes in the former
> case?
> 
> Maybe I don't understand something, but I don't see how it is not quite bad
> to expect userspace to clean up the kernel structures after the previous
> userspace client.

That's not different from the compositor just freezing instead of
crashing: Screen contents stays on and nothing happens. Imo this really is
all just broken userspace, and the kernel can't make sure userspace
doesn't randomly fall over.

What we need to make sure is that assuming things work ok-ish there's no
observed regression. And I still think that's the case here.

> What happens if something keeps crashing leaving framebuffers around?

Only the active ones would be kept around, we still clean up everything
else. So the leak is very limited from a memory pov.

> If the only reason is to avoid modeset, why SETPLANE with NULL fb to disable
> planes associated with a framebuffers to be released wouldn't work?

Because in general drivers don't support that - primary plane helpers
cant' do that and for many drivers that's the only thing we have.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 09/10/2015 10:56 AM, Daniel Vetter wrote:
> On Thu, Sep 10, 2015 at 10:07:41AM +0100, Tvrtko Ursulin wrote:
>>
>> On 09/09/2015 08:06 PM, Daniel Vetter wrote:
>>> On Wed, Sep 9, 2015 at 6:36 PM, Tvrtko Ursulin
>>> <tvrtko.ursulin@linux.intel.com> wrote:
>>>> I am not even going that far, just talking about last frame stuck on screen.
>>>> For me making that easier is a regression.
>>>
>>> So let's look at various systems:
>>> - super-modern fbdev less system: logind keeps a dup of every
>>> master-capabel drm fd. Compositor crashing won't ever result in
>>> close() getting called since logind still has its copy. Cleanup needs
>>> to be done manually anyway with the system compositor.
>>> - Current systems: Compositor restarts and cleans up the mess we left behind.
>>
>> What if the compositor doesn't restart? Or logind crashes in the former
>> case?
>>
>> Maybe I don't understand something, but I don't see how it is not quite bad
>> to expect userspace to clean up the kernel structures after the previous
>> userspace client.
>
> That's not different from the compositor just freezing instead of
> crashing: Screen contents stays on and nothing happens. Imo this really is
> all just broken userspace, and the kernel can't make sure userspace
> doesn't randomly fall over.
>
> What we need to make sure is that assuming things work ok-ish there's no
> observed regression. And I still think that's the case here.

I would disagree on the no regressions when things work okay-ish 
principle, there should be no regressions in the pessimistic scenario 
when security is concerned.

If we can agree the stuck frame on screen is not desirable from the 
security point of view, then this change does enlarge the attack surface.

Because, apart from freezing the compositor, it now also works to crash 
it and prevent restart. Maybe it is far fetched, but as I said, 
attackers have much better imagination with these things.

So for me changes like this one shouldn't be pushed in easily.

>> What happens if something keeps crashing leaving framebuffers around?
>
> Only the active ones would be kept around, we still clean up everything
> else. So the leak is very limited from a memory pov.
>
>> If the only reason is to avoid modeset, why SETPLANE with NULL fb to disable
>> planes associated with a framebuffers to be released wouldn't work?
>
> Because in general drivers don't support that - primary plane helpers
> cant' do that and for many drivers that's the only thing we have.

Could that be extended so that primary plane helpers would try to 
disable planes for which framebuffers need to be removed?

Then drivers who can't disable planes keep doing a modeset and the ones 
that can just disable planes and correctly clean up framebuffers?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[David Herrmann]

Hi

On Thu, Sep 10, 2015 at 12:15 PM, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
> On 09/10/2015 10:56 AM, Daniel Vetter wrote:
>> That's not different from the compositor just freezing instead of
>> crashing: Screen contents stays on and nothing happens. Imo this really is
>> all just broken userspace, and the kernel can't make sure userspace
>> doesn't randomly fall over.
>>
>> What we need to make sure is that assuming things work ok-ish there's no
>> observed regression. And I still think that's the case here.
>
>
> I would disagree on the no regressions when things work okay-ish principle,
> there should be no regressions in the pessimistic scenario when security is
> concerned.
>
> If we can agree the stuck frame on screen is not desirable from the security
> point of view, then this change does enlarge the attack surface.
>
> Because, apart from freezing the compositor, it now also works to crash it
> and prevent restart. Maybe it is far fetched, but as I said, attackers have
> much better imagination with these things.

I'd much more prefer a FB flag that forces a modeset on ID removal.
Anyone who cares for it can set it, and the kernel will make sure to
never keep such FBs around. However, for most use-cases, we want the
FB to stay around after close() or FB removal.

Btw., I also don't see the attack scenario at all. If an attacker
makes your compositor crash at the same moment a security-relevant
information is shown on screen, then the information is already
visible. Who cares that it stays visible? Sure, I can make up an
hypothetical use-case where that matters, but I cannot think of
something real.
Also, if the hypothetical scenario we go for is "if the compositor
crashes", then I guess there's bigger problems than the FB content.

Thanks
David
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 09/22/2015 03:53 PM, David Herrmann wrote:
> Hi
>
> On Thu, Sep 10, 2015 at 12:15 PM, Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>> On 09/10/2015 10:56 AM, Daniel Vetter wrote:
>>> That's not different from the compositor just freezing instead of
>>> crashing: Screen contents stays on and nothing happens. Imo this really is
>>> all just broken userspace, and the kernel can't make sure userspace
>>> doesn't randomly fall over.
>>>
>>> What we need to make sure is that assuming things work ok-ish there's no
>>> observed regression. And I still think that's the case here.
>>
>>
>> I would disagree on the no regressions when things work okay-ish principle,
>> there should be no regressions in the pessimistic scenario when security is
>> concerned.
>>
>> If we can agree the stuck frame on screen is not desirable from the security
>> point of view, then this change does enlarge the attack surface.
>>
>> Because, apart from freezing the compositor, it now also works to crash it
>> and prevent restart. Maybe it is far fetched, but as I said, attackers have
>> much better imagination with these things.
>
> I'd much more prefer a FB flag that forces a modeset on ID removal.
> Anyone who cares for it can set it, and the kernel will make sure to
> never keep such FBs around. However, for most use-cases, we want the
> FB to stay around after close() or FB removal.
>
> Btw., I also don't see the attack scenario at all. If an attacker
> makes your compositor crash at the same moment a security-relevant
> information is shown on screen, then the information is already
> visible. Who cares that it stays visible? Sure, I can make up an
> hypothetical use-case where that matters, but I cannot think of
> something real.
> Also, if the hypothetical scenario we go for is "if the compositor
> crashes", then I guess there's bigger problems than the FB content.

It allows losing control of the sensitive information in a new way and 
so by definition results in a less secure system.

Apparently for the goal of less flicker and easier userspace 
programming. Ie. avoiding the need for a back channel, apart from the 
fact back channel has now just been moved to the kernel.

I would recommend passing this by some more security conscious eyes.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Vincent ABRIOU]

On 09/22/2015 05:21 PM, Tvrtko Ursulin wrote:
>
> On 09/22/2015 03:53 PM, David Herrmann wrote:
>> Hi
>>
>> On Thu, Sep 10, 2015 at 12:15 PM, Tvrtko Ursulin
>> <tvrtko.ursulin@linux.intel.com> wrote:
>>> On 09/10/2015 10:56 AM, Daniel Vetter wrote:
>>>> That's not different from the compositor just freezing instead of
>>>> crashing: Screen contents stays on and nothing happens. Imo this really is
>>>> all just broken userspace, and the kernel can't make sure userspace
>>>> doesn't randomly fall over.
>>>>
>>>> What we need to make sure is that assuming things work ok-ish there's no
>>>> observed regression. And I still think that's the case here.
>>>
>>>
>>> I would disagree on the no regressions when things work okay-ish principle,
>>> there should be no regressions in the pessimistic scenario when security is
>>> concerned.
>>>
>>> If we can agree the stuck frame on screen is not desirable from the security
>>> point of view, then this change does enlarge the attack surface.
>>>
>>> Because, apart from freezing the compositor, it now also works to crash it
>>> and prevent restart. Maybe it is far fetched, but as I said, attackers have
>>> much better imagination with these things.
>>
>> I'd much more prefer a FB flag that forces a modeset on ID removal.
>> Anyone who cares for it can set it, and the kernel will make sure to
>> never keep such FBs around. However, for most use-cases, we want the
>> FB to stay around after close() or FB removal.
>>
>> Btw., I also don't see the attack scenario at all. If an attacker
>> makes your compositor crash at the same moment a security-relevant
>> information is shown on screen, then the information is already
>> visible. Who cares that it stays visible? Sure, I can make up an
>> hypothetical use-case where that matters, but I cannot think of
>> something real.
>> Also, if the hypothetical scenario we go for is "if the compositor
>> crashes", then I guess there's bigger problems than the FB content.
>
> It allows losing control of the sensitive information in a new way and
> so by definition results in a less secure system.
>
> Apparently for the goal of less flicker and easier userspace
> programming. Ie. avoiding the need for a back channel, apart from the
> fact back channel has now just been moved to the kernel.
>
> I would recommend passing this by some more security conscious eyes.
>

I made some test using weston and modetest.

When testing planes, and switching from weston to modetest, the plane 
activated with modetest is then displayed in weston and vice-versa. We 
can overcome this by updating the middleware (I only test it with 
modetest for now) to force them to properly disable the CRTC and plane 
when closed properly (I don't speak about crash). The middlewares only 
relies on drmModeRmFB to close CRTC or planes. But with this patch it 
will break the habits.
Think is will be better to update the middlewares before going futher in 
that patch direction.

Regards
Vincent
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Wed, Sep 09, 2015 at 04:40:56PM +0200, 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.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

I think the current behaviour was just a side-effect of the original
implementation and not too intentional - with no refcounting for fbs they
_had_ to be synchronously reaped. And when I've done the conversion to
refcounting I kept this to avoid gettting tangled up in an ABI-change
mess.

But I don't the current behaviour makes much sense and worth an attemp to
rectify it. And as long as we still clear the fb ids there's no real
information leak either.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

But I do want other people's opinion before I pull this in.
-Daniel

> ---
>  drivers/gpu/drm/drm_crtc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 9b9c4b41422a..626b0a57efbf 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3327,7 +3327,7 @@ int drm_mode_rmfb(struct drm_device *dev,
>  	mutex_unlock(&dev->mode_config.fb_lock);
>  	mutex_unlock(&file_priv->fbs_lock);
>  
> -	drm_framebuffer_remove(fb);
> +	drm_framebuffer_unreference(fb);
>  
>  	return 0;
>  
> @@ -3517,7 +3517,7 @@ void drm_fb_release(struct drm_file *priv)
>  		list_del_init(&fb->filp_head);
>  
>  		/* This will also drop the fpriv->fbs reference. */
> -		drm_framebuffer_remove(fb);
> +		drm_framebuffer_unreference(fb);
>  	}
>  }
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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

[David Herrmann]

Hi

On Wed, Sep 9, 2015 at 5:02 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Sep 09, 2015 at 04:40:56PM +0200, 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.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> I think the current behaviour was just a side-effect of the original
> implementation and not too intentional - with no refcounting for fbs they
> _had_ to be synchronously reaped. And when I've done the conversion to
> refcounting I kept this to avoid gettting tangled up in an ABI-change
> mess.
>
> But I don't the current behaviour makes much sense and worth an attemp to
> rectify it. And as long as we still clear the fb ids there's no real
> information leak either.
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> But I do want other people's opinion before I pull this in.

We _REALLY_ want this! It makes a lot of our life much easier when
trying to get rid of flicker during boot-up. It currently sucks that
neither the boot-loader screen, nor the boot-splash screen, nor the
login-manager screen can be left around after they quit and handover
to the next stage. We have to stay around for hand-over, which is
nasty and requires a back-channel which is otherwise not needed at
all.

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/core: Preserve the fb id on close.

[Maarten Lankhorst]

Keep the fb_id, which means that any application exiting without
unsetting the framebuffer from all planes will preserve its contents.

This is similar to preserving the initial framebuffer, except all
planes are preserved.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 626b0a57efbf..9d55c0c6aa95 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3320,9 +3320,6 @@ int drm_mode_rmfb(struct drm_device *dev,
 	if (!found)
 		goto fail_lookup;
 
-	/* Mark fb as reaped, we still have a ref from fpriv->fbs. */
-	__drm_framebuffer_unregister(dev, fb);
-
 	list_del_init(&fb->filp_head);
 	mutex_unlock(&dev->mode_config.fb_lock);
 	mutex_unlock(&file_priv->fbs_lock);
@@ -3508,15 +3505,9 @@ void drm_fb_release(struct drm_file *priv)
 	 * at it any more.
 	 */
 	list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) {
-
-		mutex_lock(&dev->mode_config.fb_lock);
-		/* Mark fb as reaped, we still have a ref from fpriv->fbs. */
-		__drm_framebuffer_unregister(dev, fb);
-		mutex_unlock(&dev->mode_config.fb_lock);
-
 		list_del_init(&fb->filp_head);
 
-		/* This will also drop the fpriv->fbs reference. */
+		/* This drops the fpriv->fbs reference. */
 		drm_framebuffer_unreference(fb);
 	}
 }
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/core: Preserve the fb id on close.

[David Herrmann]

Hi

On Wed, Sep 9, 2015 at 4:40 PM, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> Keep the fb_id, which means that any application exiting without
> unsetting the framebuffer from all planes will preserve its contents.
>
> This is similar to preserving the initial framebuffer, except all
> planes are preserved.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)

Same as 1/2:

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 626b0a57efbf..9d55c0c6aa95 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3320,9 +3320,6 @@ int drm_mode_rmfb(struct drm_device *dev,
>         if (!found)
>                 goto fail_lookup;
>
> -       /* Mark fb as reaped, we still have a ref from fpriv->fbs. */
> -       __drm_framebuffer_unregister(dev, fb);
> -
>         list_del_init(&fb->filp_head);
>         mutex_unlock(&dev->mode_config.fb_lock);
>         mutex_unlock(&file_priv->fbs_lock);
> @@ -3508,15 +3505,9 @@ void drm_fb_release(struct drm_file *priv)
>          * at it any more.
>          */
>         list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) {
> -
> -               mutex_lock(&dev->mode_config.fb_lock);
> -               /* Mark fb as reaped, we still have a ref from fpriv->fbs. */
> -               __drm_framebuffer_unregister(dev, fb);
> -               mutex_unlock(&dev->mode_config.fb_lock);
> -
>                 list_del_init(&fb->filp_head);
>
> -               /* This will also drop the fpriv->fbs reference. */
> +               /* This drops the fpriv->fbs reference. */
>                 drm_framebuffer_unreference(fb);
>         }
>  }
> --
> 2.1.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx