On Mon, Feb 10, 2020 at 8:01 PM Emil Velikov <emil.l.velikov@xxxxxxxxx> wrote: > > Thanks for having a look Daniel. > > On Fri, 7 Feb 2020 at 13:29, Daniel Vetter <daniel@xxxxxxxx> wrote: > > > > On Wed, Feb 05, 2020 at 05:48:39PM +0000, Emil Velikov wrote: > > > From: Emil Velikov <emil.velikov@xxxxxxxxxxxxx> > > > > > > This commit reworks the permission handling of the two ioctls. In > > > particular it enforced the CAP_SYS_ADMIN check only, if: > > > - we're issuing the ioctl from process other than the one which opened > > > the node, and > > > - we are, or were master in the past > > > > > > This allows for any application which cannot rely on systemd-logind > > > being present (for whichever reason), to drop it's master capabilities > > > (and regain them at later point) w/o being ran as root. > > > > > > See the comment above drm_master_check_perm() for more details. > > > > > > Cc: Adam Jackson <ajax@xxxxxxxxxx> > > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx> > > > Signed-off-by: Emil Velikov <emil.velikov@xxxxxxxxxxxxx> > > > --- > > > This effectively supersedes an earlier patch [1] incorporating ajax's > > > feedback (from IRC ages ago). > > > > > > [1] https://patchwork.freedesktop.org/patch/268977/ > > > --- > > > drivers/gpu/drm/drm_auth.c | 59 +++++++++++++++++++++++++++++++++++++ > > > drivers/gpu/drm/drm_ioctl.c | 4 +-- > > > include/drm/drm_file.h | 11 +++++++ > > > 3 files changed, 72 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c > > > index cc9acd986c68..01d9e35c0106 100644 > > > --- a/drivers/gpu/drm/drm_auth.c > > > +++ b/drivers/gpu/drm/drm_auth.c > > > @@ -135,6 +135,7 @@ static int drm_set_master(struct drm_device *dev, struct drm_file *fpriv, > > > } > > > } > > > > > > + fpriv->was_master = (ret == 0); > > > return ret; > > > } > > > > > > @@ -179,12 +180,64 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv) > > > return ret; > > > } > > > > > > +/* > > > + * In the olden days the SET/DROP_MASTER ioctls used to return EACCES when > > > + * CAP_SYS_ADMIN was not set. > > > + * > > > + * Even though the first client is _always_ master, it also had to be run as > > > + * root, otherwise SET/DROP_MASTER would fail. In those cases no other client > > > + * could become master ... EVER. > > > + * > > > + * Resulting in a) the graphics session dying badly or b) a completely locked > > > + * session :-\ > > > + * > > > + * As some point systemd-logind was introduced to orchestrate and delegate > > > + * master as applicable. It does so by opening the fd and passing it to users > > > + * while in itself logind a) set/drop master per users' request and b) > > > + * implicitly drops master on VT switch. > > > + * > > > + * Even though logind looks like the future, there are a few obstacles: > > > + * - using it is not possible on some platforms, or > > > + * - applications may not be updated to use it, > > > + * - any client which fails to drop master* can DoS the application using > > > + * logind, to a varying degree. > > > + * > > > + * * Either due missing root permission or simply not calling DROP_MASTER. > > > + * > > > + * > > > + * Here we implement the next best thing: > > > + * We enforce the CAP_SYS_ADMIN check only if the client was not a master > > > + * before. We distinguish between the original master client (say logind) and > > > + * another client which has the fd passed (say Xorg) by comparing the pids. > > > + * > > > + * As a result this fixes, the following when using root-less build w/o logind > > > + * - startx - some drivers work fine regardless > > > + * - weston > > > + * - various compositors based on wlroots > > > + */ > > > > I think this breaks logind security. With logind no compositor can open > > the device node directly, hence no compositor can accidentally become the > > master and block everyone else. > > > I've explicitly considered this case. AFAICT this patch does not > change any of the contract. > If you think there's a scenario where things have broken, please let me know. > > > And for the vt switch logind is the only one that can grant master rights, > > and it can make sure that the right compositor gets them. And if the old > > compositor is non-cooperating, it can also forcefully remove master > > rights. > > > Yes logind does set/drop master on VT switch, session setup/teardown, etc. > > To take this a step further, there is no logind API or dbus method for > compositors to only set/drop master. > Thus logind ensures that compositors are in sane state. > > > But with this here we lift this restriction if a compositor has ever been > > master. So the following thing could happen: > > - We have 3 compositors for different users C1, C2, C3 > > - We switch from C1 to C2 > > - While we switch no one is master for a moment, which means C3 could > > sneak in and do a quick setmaster, and become master > > - Everything would come crashing done since logind believes it already > > revoked master for C1, but somehow it now cant grant master to C2 > > > Does this scenario consider that all three compositors are logind users? > If so, C3 should not be able to set or drop master. Since it got its > fd from logind: > > - `file_priv->pid` will point to systemd-logind, and > - `task_pid(current)` will point to the respective compositor > > -> EACCES will be returned to any compositor calling drmSetMaster. > > Regardless of my patch, C3 can open() and simply not release the master. > Assuming it's the first DRM client of course - say switch to VTx + > login + start C3. Hm ... I guess this works indeed. Or should. I'm mildly freaked out that we're checking for opener_pid == current->pid. Not sure how many other security assumptions we're breaking. > I've been lucky enough to spot various ways to softlock my system... > even when the compositor is using logind ;-) > If you're really interested I can share, but I'm worried that people > may see them as bashing at logind. > > > I'm not sure we can even support these two models at the same time. > > > > > +static int > > > +drm_master_check_perm(struct drm_device *dev, struct drm_file *file_priv) > > > +{ > > > + if (file_priv->pid != task_pid(current) && file_priv->was_master) > > > > Isn't this a typo? Why should we only allow this if the opener is someone > > else ... that looks like the logind approach? Or is my bolean logic parser > > broken again. > > > Thanks for spotting it. Indeed that should be: > > if (file_priv->pid == task_pid(current) && file_priv->was_master) > return 0; > > > Modulo any objections, I'll do proper testing and submit a non RFC version. > The inline comments will explicitly mention your concerns and why the > patch is safe. Given the above bug I think a solid igt for both the logind and the non-logind scenario is needed. We have some helpers to drop root and fork stuff and all that, so shouldn't be many lines. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
