| Message ID | 1441809657-11411-2-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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); } }
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(-)