[PATCH] drm: Fix an unwanted master inheritance

[Daniel Vetter]

On Mon, Nov 30, 2015 at 04:27:50PM +0100, Thomas Hellstrom wrote:
> Hi,
> 
> On 11/30/2015 04:00 PM, Daniel Vetter wrote:
> > On Mon, Nov 30, 2015 at 04:44:21AM -0800, Thomas Hellstrom wrote:
> >> A client calling drmSetMaster() using a file descriptor that was opened
> >> when another client was master would inherit the latter client's master
> >> object and all it's authenticated clients.
> >>
> >> This is unwanted behaviour, and when this happens, instead allocate a
> >> brand new master object for the client calling drmSetMaster().
> >>
> >> Signed-off-by: Thomas Hellstrom <thellstrom at vmware.com>
> > Imo makes sense. It would be great to have a testcase for this, and for
> > non-kms stuff igt now has support for generic testcases that can be run on
> > any driver. See for example intel-gpu-tools/tests/core_get_auth_client.c.
> >
> > I or Daniel Stone can help out (on irc or mail) with that.
> > -Daniel
> 
> Given that this crashes the kernel by vmwgfx throwing a BUG on some
> versions of SLE,
> while probably all other drivers don't care, except that it's a security
> issue, A generic test case involving DRM clients leaking information
> between master realms would unfortunately be too resource consuming to
> put together for our minimal driver team ;).
> 
> Although I used the attached C program run as root to trigger the
> behavior and unconditional kernel crash on vmwgfx. On the affected SLE
> versions, fd1 would represent Xorg, fd2 would represent plymouthd.
> 
> /Thomas
> 

> #include <xf86drm.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <unistd.h>
> #include <stdlib.h>
> #include <stdio.h>
> 
> int main()
> {
>     int fd1, fd2;
> 
>     fd1 = open("/dev/dri/card0", O_RDWR);
>     if (fd1 < 0)
> 	exit(-1);
> 
>     fd2 = open("/dev/dri/card0", O_RDWR);
>     if (fd2 < 0)
> 	exit(-1);

I think if you open fd3 here an auth it with fd1 ...

>     (void) drmDropMaster(fd1);
>     (void) drmSetMaster(fd2);

and then check whether fd1 is still authenticated (and fail if so) it
should work as a testcase. Converting it over to igt would be trivial, I
can do that if you want. We also already have auth testcases in igt, so
should be at most a bit of copypasting to get it together.

Or did I miss a needed detail in how to repro this?
-Daniel

> 
>     close(fd2);
>     close(fd1);
> }


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