On 25/08/2013 17:09, David Herrmann wrote: > 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? ;)
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
