On Tue, 11 Feb 2020 at 08:06, Pekka Paalanen ppaalanen@gmail.com wrote:
On Mon, 10 Feb 2020 19:01:06 +0000 Emil Velikov emil.l.velikov@gmail.com wrote:
Thanks for having a look Daniel.
On Fri, 7 Feb 2020 at 13:29, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Feb 05, 2020 at 05:48:39PM +0000, Emil Velikov wrote:
From: Emil Velikov emil.velikov@collabora.com
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@redhat.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Emil Velikov emil.velikov@collabora.com
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
+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;
Hi,
I'm mostly just curious, why is comparing pids safe here? Maybe the 'pid' member is not what userspace calls PID?
PID here is the kernel struct pid. For userspace ones we have the distinct task_xid_nr, task_xid_vnr and task_xid_nr_ns. See the documentation [1] for details.
What if a malicious process receives a DRM fd from something similar to logind, then the logind equivalent process dies,
In the logind case, systemd ensures to bring it back up ASAP. For others - shrug?
and the malicious process starts forking new processes attempting to hit the same pid the logind equivalent had, succeeds in that, and passes the DRM fd to that fork. Is the fork then effectively in control of DRM master?
Valid point, although I believe we're covered.
First and foremost, the pid we store is refcounted [1]. So in order for this to happen we need have both a) a pretty fundamental refcount bug for the pid to gets destroyed and b) we need to allocate another one at the exact same address.
Individually - pretty unlikely, combined - beyond paranoid IMHO.
Additionally, today there are other ways to cause issues. In particular: - logind dies - the application already has an fd (from logind) - the fd is master capable - application is free to do as it wishes ... apart from dropping master (oh noo) and setting it back up again
Or a simple application which loops over open() + drmIsMaster() + close(). There are others, although I'd be going pretty much off-topic.
Thanks Emil
[1] https://elixir.bootlin.com/linux/v5.5/source/include/linux/sched.h#L1307 [2] https://elixir.bootlin.com/linux/v5.5/source/drivers/gpu/drm/drm_file.c#L127