[RFC] drm: rework SET_MASTER and DROP_MASTER perm handling

[Pekka Paalanen]

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?

What if a malicious process receives a DRM fd from something similar to
logind, then the logind equivalent process dies, 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?


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200211/7de1107b/attachment.sig>