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