On Wed, Aug 06, 2014 at 07:17:25PM +0200, Christian K?nig wrote: > Am 06.08.2014 um 18:08 schrieb Jerome Glisse: > >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 at amd.com> > >>>>>> > >>>>>>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. > > We check the access rights of the pointer using: > > if (!access_ok(write ? VERIFY_WRITE : VERIFY_READ, > >(long)gtt->userptr, > > ttm->num_pages * PAGE_SIZE)) > > return -EFAULT; > > Shouldn't that be enough?
No, access_ok only check against special area on some architecture and i am pretty sure on x86 the VERIFY_WRITE or VERIFY_READ is just flat out ignored. What you need to test is the vma vm_flags somethings like if (write && !(vma->vm_flags VM_WRITE)) return -EPERM; Which need to happen on all bind. Cheers, J?r?me > > Christian. > > > > >Cheers, > >J?r?me >
