[RFC] drm: rework SET_MASTER and DROP_MASTER perm handling

[Daniel Vetter]

On Tue, Feb 11, 2020 at 11:46:26AM +0000, Emil Velikov wrote:
> 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.

Yeah, the kernel-internal pid structure maps to the shiny new pidfd stuff,
not to traditional unix pid numbers with all their problems around races
and reuse when there's not a parent/child relationship.
-Daniel

> 
> 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch