[PATCH] drm: Fix an unwanted master inheritance

[Thomas Hellstrom]

On 11/30/2015 05:09 PM, Daniel Vetter wrote:
> 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

Yes, an authenticated fd is always authenticated, no matter what master
is current. And core DRM leaves data isolation between master realms to
subsystems or drivers.

What we could do is to obtain an auth magic for fd3 from fd1 and try to
use it with fd2 to authenticate after master switch. That should work
without this patch, but not with is.

/Thomas


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