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