Re: [PATCH v2 0/6] DRM: VMA Access Management and Render Nodes

[Martin Peres <martin.peres@free.fr>]

On 25/08/2013 17:09, David Herrmann wrote:
> Hi
>
> On Fri, Aug 23, 2013 at 2:00 PM, Martin Peres <martin.peres@free.fr> wrote:
>> Le 23/08/2013 13:13, David Herrmann a écrit :
>>
>>> Hi
>>>
>>> I reduced the vma access-management patches to a minimum. I now do filp*
>>> tracking in gem unconditionally and force drm_gem_mmap() to check this.
>>> Hence,
>>> all gem drivers are safe now. For TTM drivers, I now use the already
>>> available
>>> verify_access() callback to get access to the underlying gem-object.
>>> Pretty
>>> simple.. Why hadn't I thought of that before?
>>>
>>> Long story short: All drivers using GEM are safe now. This leaves vmwgfx..
>>> But
>>> they do their own access-management, anyway.
>>
>> Great! Thanks! Have you checked they are really safe with my "exploits"?
>> I'll have another round of review even if it looked good the last time I
>> checked.
> Good you asked. I tested whether it works, I didn't actually verify
> that it correctly fails in case of exploits. And in fact there is a
> small bug (I return "1" instead of -EACCES, stupid verify_access()) so
> user-space gets a segfault accessing the mmap when trying to exploit
> this. That actually doesn't sound that bad, does it? ;)

Good to know I could contribute a little to your work. You're doing a 
great job!
> v2 is on its way.

Yep, saw it.
>
>>> The 3 patches on top implement render-nodes. I added a "drm_rnodes" module
>>> parameter to core drm. You need to pass "drm.rnodes=1" on the kernel
>>> command-line or via sysfs _before_ loading a driver. Otherwise, render
>>> nodes
>>> will not be created.
>>
>> By default, having the render nodes doesn't change the way the userspace
>> works at all. So, what is the point of protecting it behind a parameter?
>>
>> Is it to make it clear this isn't part of the API yet? I would say that as
>> long
>> as libdrm hasn't been updated, this isn't part of the API anyway.
> Hm, I wouldn't say so. Applications like weston and kmscon no longer
> use the legacy drmOpen() facility. They use udev+open(). So once it's
> upstream, it's part of the API regardless of libdrm. So the sole
> purpose of drm_rnodes is to mark it as "experimental".

Ah, I guess I'll have to have a look at this. I basically got preempted
from adding render node support to Weston and I didn't take the time
to check it again, the vma patches were more important first. Thanks
for saving me a giant headache with GEM/TTM, I spent two week ends
trying to track a leak for radeon cards.
>>> This allows us to test render-nodes and play with the API. I added FLINK
>>> for
>>> now so we can better test it. Not sure whether we should allow it in the
>>> end,
>>> though.
>>
>>  From a security point of view, I don't think we should keep it as
>> applications shouldn't
>> be trusted for not doing stupid things (because of code injection). So,
>> unless we
>> plan on adding access control to flink via LSM, we shouldn't expose the
>> feature
>> on render nodes.
> This is also what I think. We have a chance to get rid of all legacy
> stuff, so maybe we should just drop it all.

Great!
>
>>  From a dev point of view, keeping it means that the XServer doesn't
>> have to know whether mesa supports render nodes or not. This is because
>> the authentication dance isn't available on render nodes so the x-server
>> has to tell mesa if it should authenticate or not. The other solution is to
>> allow
>> the authentication ioctls on render nodes and just return 0 if this is a
>> render node.
>> This way, we won't need any modification in mesa/xserver/dri2proto to pass
>> the information that no authentication is needed. In this solution, only
>> libdrm and
>> the ddx should be modified to make use of the render node. That's not how I
>> did it on my render node patchset, can't remember why...
>>
>> What do you guys think?
> We discussed that a bit on IRC. Of course, we can add a lot of
> wrappers and workarounds. We can make all the drmAuth stuff *just
> work*. But that means, we keep all the legacy. As said, we have the
> chance to introduce a new API and drop all the legacy. I think it is
> worth a shot. And we also notice quite fast which user-space programs
> need some rework.

Well, if by "all the legacy", you mean the authentication-related 
functions then yes.

How do you plan on handling the case where the ddx has been updated and 
passes
the render node to a not-yet-updated mesa? Mesa will try to authenticate 
and it will fail.

Keeping the authentication IOCTLs seem to me like a lesser evil, 
especially since they
would basically do nothing.
>
>>> Maybe we can get this into 3.11?
>>
>> As long as we don't have to keep the interface stable (I don't want to
>> expose flink
>> on render nodes), I'm all for pushing the code now. Otherwise, a kernel
>> branch
>> somewhere is sufficient.
>>
>> Do you plan on checking my userspace patches too? Those are enough to make
>> use
>> of the render nodes on X, but I haven't tested that all the combinations of
>> version
>> would still work. The libdrm work should be quite solid though (there are
>> even libdrm
>> tests for the added functionalities :)).
> I plan on having a working user-space for XDC. Most of your patches
> can be copied unchanged indeed. But servers other than Xorg don't use
> that, so they need separate fixes.
Brilliant :) Looking forward to it!

Martin