Re: [RFC] drm: rework SET_MASTER and DROP_MASTER perm handling

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2738 bytes --]

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?

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

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel