[RFC] drm: rework SET_MASTER and DROP_MASTER perm handling

[Emil Velikov]

On Tue, 11 Feb 2020 at 08:06, Pekka Paalanen <ppaalanen at gmail.com> wrote:
>
> On Mon, 10 Feb 2020 19:01:06 +0000
> Emil Velikov <emil.l.velikov at gmail.com> wrote:
>
> > Thanks for having a look Daniel.
> >
> > On Fri, 7 Feb 2020 at 13:29, Daniel Vetter <daniel at ffwll.ch> wrote:
> > >
> > > On Wed, Feb 05, 2020 at 05:48:39PM +0000, Emil Velikov wrote:
> > > > From: Emil Velikov <emil.velikov at 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 at redhat.com>
> > > > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > > Signed-off-by: Emil Velikov <emil.velikov at 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