kernel
Revision | ebf8b0462c25da747c0c0c1ed03941b46852fa7c (tree) |
---|---|
Zeit | 2020-05-12 12:43:22 |
Autor | Emil Velikov <emil.velikov@coll...> |
Commiter | Chih-Wei Huang |
drm: rework SET_MASTER and DROP_MASTER perm handling
This commit reworks the permission handling of the two ioctls. In
particular it enforced the CAP_SYS_ADMIN check only, if:
the node, and
This ensures that we:
master capabilities (and regain them at later point) ... w/o running as
root.
See the comment above drm_master_check_perm() for more details.
v1:
v2:
Cc: Adam Jackson <ajax@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Testcase: igt/core_setmaster/master-drop-set-user
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
Reviewed-by: Adam Jackson <ajax@redhat.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20200319172930.230583-1-emil.l.velikov@gmail.com
@@ -135,6 +135,7 @@ static int drm_set_master(struct drm_device *dev, struct drm_file *fpriv, | ||
135 | 135 | } |
136 | 136 | } |
137 | 137 | |
138 | + fpriv->was_master = (ret == 0); | |
138 | 139 | return ret; |
139 | 140 | } |
140 | 141 |
@@ -179,12 +180,72 @@ out_err: | ||
179 | 180 | return ret; |
180 | 181 | } |
181 | 182 | |
183 | +/* | |
184 | + * In the olden days the SET/DROP_MASTER ioctls used to return EACCES when | |
185 | + * CAP_SYS_ADMIN was not set. This was used to prevent rogue applications | |
186 | + * from becoming master and/or failing to release it. | |
187 | + * | |
188 | + * At the same time, the first client (for a given VT) is _always_ master. | |
189 | + * Thus in order for the ioctls to succeed, one had to _explicitly_ run the | |
190 | + * application as root or flip the setuid bit. | |
191 | + * | |
192 | + * If the CAP_SYS_ADMIN was missing, no other client could become master... | |
193 | + * EVER :-( Leading to a) the graphics session dying badly or b) a completely | |
194 | + * locked session. | |
195 | + * | |
196 | + * | |
197 | + * As some point systemd-logind was introduced to orchestrate and delegate | |
198 | + * master as applicable. It does so by opening the fd and passing it to users | |
199 | + * while in itself logind a) does the set/drop master per users' request and | |
200 | + * b) * implicitly drops master on VT switch. | |
201 | + * | |
202 | + * Even though logind looks like the future, there are a few issues: | |
203 | + * - some platforms don't have equivalent (Android, CrOS, some BSDs) so | |
204 | + * root is required _solely_ for SET/DROP MASTER. | |
205 | + * - applications may not be updated to use it, | |
206 | + * - any client which fails to drop master* can DoS the application using | |
207 | + * logind, to a varying degree. | |
208 | + * | |
209 | + * * Either due missing CAP_SYS_ADMIN or simply not calling DROP_MASTER. | |
210 | + * | |
211 | + * | |
212 | + * Here we implement the next best thing: | |
213 | + * - ensure the logind style of fd passing works unchanged, and | |
214 | + * - allow a client to drop/set master, iff it is/was master at a given point | |
215 | + * in time. | |
216 | + * | |
217 | + * Note: DROP_MASTER cannot be free for all, as an arbitrator user could: | |
218 | + * - DoS/crash the arbitrator - details would be implementation specific | |
219 | + * - open the node, become master implicitly and cause issues | |
220 | + * | |
221 | + * As a result this fixes the following when using root-less build w/o logind | |
222 | + * - startx | |
223 | + * - weston | |
224 | + * - various compositors based on wlroots | |
225 | + */ | |
226 | +static int | |
227 | +drm_master_check_perm(struct drm_device *dev, struct drm_file *file_priv) | |
228 | +{ | |
229 | + if (file_priv->pid == task_pid(current) && file_priv->was_master) | |
230 | + return 0; | |
231 | + | |
232 | + if (!capable(CAP_SYS_ADMIN)) | |
233 | + return -EACCES; | |
234 | + | |
235 | + return 0; | |
236 | +} | |
237 | + | |
182 | 238 | int drm_setmaster_ioctl(struct drm_device *dev, void *data, |
183 | 239 | struct drm_file *file_priv) |
184 | 240 | { |
185 | 241 | int ret = 0; |
186 | 242 | |
187 | 243 | mutex_lock(&dev->master_mutex); |
244 | + | |
245 | + ret = drm_master_check_perm(dev, file_priv); | |
246 | + if (ret) | |
247 | + goto out_unlock; | |
248 | + | |
188 | 249 | if (drm_is_current_master(file_priv)) |
189 | 250 | goto out_unlock; |
190 | 251 |
@@ -229,6 +290,12 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data, | ||
229 | 290 | int ret = -EINVAL; |
230 | 291 | |
231 | 292 | mutex_lock(&dev->master_mutex); |
293 | + | |
294 | + ret = drm_master_check_perm(dev, file_priv); | |
295 | + if (ret) | |
296 | + goto out_unlock; | |
297 | + | |
298 | + ret = -EINVAL; | |
232 | 299 | if (!drm_is_current_master(file_priv)) |
233 | 300 | goto out_unlock; |
234 | 301 |
@@ -601,8 +601,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = { | ||
601 | 601 | DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_SET_SAREA_CTX, drm_legacy_setsareactx, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), |
602 | 602 | DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_GET_SAREA_CTX, drm_legacy_getsareactx, DRM_AUTH), |
603 | 603 | |
604 | - DRM_IOCTL_DEF(DRM_IOCTL_SET_MASTER, drm_setmaster_ioctl, DRM_ROOT_ONLY), | |
605 | - DRM_IOCTL_DEF(DRM_IOCTL_DROP_MASTER, drm_dropmaster_ioctl, DRM_ROOT_ONLY), | |
604 | + DRM_IOCTL_DEF(DRM_IOCTL_SET_MASTER, drm_setmaster_ioctl, 0), | |
605 | + DRM_IOCTL_DEF(DRM_IOCTL_DROP_MASTER, drm_dropmaster_ioctl, 0), | |
606 | 606 | |
607 | 607 | DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_ADD_CTX, drm_legacy_addctx, DRM_AUTH|DRM_ROOT_ONLY), |
608 | 608 | DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_RM_CTX, drm_legacy_rmctx, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), |
@@ -201,6 +201,17 @@ struct drm_file { | ||
201 | 201 | bool writeback_connectors; |
202 | 202 | |
203 | 203 | /** |
204 | + * @was_master: | |
205 | + * | |
206 | + * This client has or had, master capability. Protected by struct | |
207 | + * &drm_device.master_mutex. | |
208 | + * | |
209 | + * This is used to ensure that CAP_SYS_ADMIN is not enforced, if the | |
210 | + * client is or was master in the past. | |
211 | + */ | |
212 | + bool was_master; | |
213 | + | |
214 | + /** | |
204 | 215 | * @is_master: |
205 | 216 | * |
206 | 217 | * This client is the creator of @master. Protected by struct |