On Fri, 6 Mar 2020 at 14:00, Pekka Paalanen <ppaalanen@xxxxxxxxx> wrote: > > On Wed, 19 Feb 2020 13:27:28 +0000 > Emil Velikov <emil.l.velikov@xxxxxxxxx> wrote: > > > From: Emil Velikov <emil.velikov@xxxxxxxxxxxxx> > > > > ... > > > +/* > > + * In the olden days the SET/DROP_MASTER ioctls used to return EACCES when > > + * CAP_SYS_ADMIN was not set. This was used to prevent rogue applications > > + * from becoming master and/or failing to release it. > > + * > > + * At the same time, the first client (for a given VT) is _always_ master. > > + * Thus in order for the ioctls to succeed, one had to _explicitly_ run the > > + * application as root or flip the setuid bit. > > + * > > + * If the CAP_SYS_ADMIN was missing, no other client could become master... > > + * EVER :-( Leading to a) the graphics session dying badly or b) a completely > > + * locked session. > > + * > > Hi, > > sorry I had to trim this email harshly, but Google did not want to > deliver it otherwise. > > I agree that being able to drop master without CAP_SYS_ADMIN sounds > like a good thing. > > > + * > > + * 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) does the 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 issues: > > + * - using it is not possible on some platforms > > + * - 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 CAP_SYS_ADMIN or simply not calling DROP_MASTER. > > + * > > + * > > + * Here we implement the next best thing: > > + * - ensure the logind style of fd passing works unchanged, and > > + * - allow a client to drop/set master, iff it is/was master at a given point > > + * in time. > > I understand the drop master part, because it is needed to get rid of > apps that accidentally gain DRM master because they were the first one > to open the DRM device (on a particular VT?). It could happen e.g. if a > Wayland client is inspecting DRM devices to figure what it wants to > lease while the user has VT-switched to a text-mode VT, I guess. E.g. > starting a Wayland VR compositor from a VT for whatever reason. > > The set master without CAP_SYS_ADMIN part I don't understand. > As you point out application can drop master for various reasons. One of which may be to say spawn another program which requires master for _non_ modeset reasons. For example: - amdgpu: create a renderer and use the context/process priority override IOCTL - vmwgfx: stream claim/unref (amongst others). Another case to consider is classic X or Wayland compositor. With CAP_SYS_ADMIN for DROP_MASTER removed, yet retained in SET_MASTER, the IOCTL will fail. Thus: - weston results in frozen session and session switching (have to force kill weston or sudo loginctl kill-session) - depending on the driver, X will work or crash To make this clearer I'll include //comment sections in the code. // comment To ensure the application can reclaim its master status, the tweaked CAP_SYS_ADMIN handling is needed for both IOCTLs. Otherwise X or Wayland compositors may freeze or crash as SET_MASTER fails. // comment > > + * > > + * As a result this fixes, the following when using root-less build w/o logind > > Why is non-root without any logind-equivalent a use case that should > work? > // comment Some platforms don't have equivalent (Android, CrOS, some BSDs), yet root is required _solely_ for DROP/SET MASTER. So tweaking the requirement sounds perfectly reasonable. // comment > Why did DRM set/drop master use to require CAP_SYS_ADMIN in the first > place? > I imagine something else could have been introduced instead. Although I cannot find any details or discussion on the topic. > What else happens if we allow DRM set master more than just for > CAP_SYS_ADMIN? > If we're talking about removing CAP_SYS_ADMIN all together: - screen scraping by any random application - dead trivial way to DoS your compositor > Does this interact with DRM leasing? > > Looks like drmIsMaster() should be unaffected at least: > https://patchwork.kernel.org/patch/10776525/ > Correct, both are unaffected. All the leasing IOCTLs happen by the active true (aka non-lease) master. > > + * - startx - some drivers work fine regardless > > + * - weston > > + * - various compositors based on wlroots > > + */ > > +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) > > + return 0; > > In case a helper e.g. logind opens the device, is file_priv->pid then > referring to logind regardless of what happens afterwards? > Correct. > > + > > + if (!capable(CAP_SYS_ADMIN)) > > + return -EACCES; > > + > > + return 0; > > +} > > + > > int drm_setmaster_ioctl(struct drm_device *dev, void *data, > > struct drm_file *file_priv) > > { > > int ret = 0; > > > > mutex_lock(&dev->master_mutex); > > + > > + ret = drm_master_check_perm(dev, file_priv); > > + if (ret) > > + goto out_unlock; > > + > > if (drm_is_current_master(file_priv)) > > goto out_unlock; > > > > @@ -229,6 +285,12 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data, > > int ret = -EINVAL; > > > > mutex_lock(&dev->master_mutex); > > + > > + ret = drm_master_check_perm(dev, file_priv); > > Why does drop-master need any kind of permission check? Why could it > not be free for all? > Consider the arbitrator usecase - be that logind, Xorg (in ancient times) or otherwise. // comment DROP_MASTER cannot be free for all, as any (say logind) user can: - can DoS/crash the arbitrator - open the node, become master implicitly and cause issues // comment I've added an IGT subtest to ensure this does not happen. Let me know if I should include anything more to the commit, than the above comment sections. Thanks -Emil _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
