On Wed, Aug 06, 2014 at 08:55:28AM +0200, Christian König wrote: > Am 06.08.2014 um 00:13 schrieb Jerome Glisse: > >On Tue, Aug 05, 2014 at 07:45:21PM +0200, Christian König wrote: > >>Am 05.08.2014 um 19:39 schrieb Jerome Glisse: > >>>On Tue, Aug 05, 2014 at 06:05:29PM +0200, Christian König wrote: > >>>>From: Christian König <christian.koenig@xxxxxxx> > >>>> > >>>>Avoid problems with writeback by limiting userptr to anonymous memory. > >>>> > >>>>v2: add commit and code comments > >>>I guess, i have not expressed myself clearly. This is bogus, you pretend > >>>you want to avoid writeback issue but you still allow userspace to map > >>>file backed pages (which by the way might be a regular bo object from > >>>another device for instance and that would be fun). > >>> > >>>So this patch is a no go and i would rather see that this userptr to > >>>be restricted to anon vma only no matter what. No flags here. > >>Mapping of non anonymous memory (e.g. everything get_user_pages won't fail > >>with) is restricted to read only access by the GPU. > >> > >>I'm fine with making it a hard requirement for all mappings if you say it's > >>a must have. > >> > >Well for time being you should force read only. The way you implement write > >is broken. Here is how it can abuse to allow write to a file backed mmap. > > > >mmap(fixaddress,fixedsize,NOFD) > >userptr_ioctl(fixedaddress, RADEON_GEM_USERPTR_ANONONLY) > >// bo is created successfully because fixedaddress is part of anonvma > >munmap(fixedaddress,fixedsize) > >// radeon get mmu_notifier_range_start callback and unbind page from the > >// bo but radeon does not know there was an unmap. > >mmap(fixaddress,fixedsize,fd_to_this_read_only_file_i_want_to_write_to) > >radeon_ioctl_use_my_userptrbo > >// bo is bind again by radeon and because all flag are set at creation > >// it is map with write permission allowing someone to write to a file > >// that might be read only for the user. > >// > >// Script kiddies it's time to learn about gpu ... > > > >Of course if you this patch (kind of selling my own junk here) : > > > >http://www.spinics.net/lists/linux-mm/msg75878.html > > > >then you could know inside the range_start that you should remove the > >write permission and that it should be rechecked on next bind. > > > >Note that i have not read much of your code so maybe you handle this > >case somehow. > > I've stumbled over this attack vector as well and it's the reason why I've > moved checking the access rights to the bind callback instead of BO creation > time with V5 of the patch. > > This way you get an -EFAULT if you try something like this on command > submission time. So you seem immune to that issue but you are still not checking if the anon vma is writeable which you should again security concern here. Cheers, Jérôme _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
