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

[David Herrmann]

Hi

On Fri, Aug 23, 2013 at 2:00 PM, Martin Peres <martin.peres at 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? ;)

v2 is on its way.

>> 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".

>> 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.

> 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.

>>
>> 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.

Cheers
David