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? Christian. > > Cheers, > J?r?me
