Am 07.08.2014 um 08:55 schrieb Daniel Vetter: > On Wed, Aug 06, 2014 at 11:45:48PM -0400, Jerome Glisse wrote: >> On Wed, Aug 06, 2014 at 10:24:31PM +0200, Daniel Vetter wrote: >>> On Wed, Aug 06, 2014 at 02:34:16PM -0400, Jerome Glisse wrote: >>>> 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. That seems to be unnecessary, since get_user_pages will check that for us anyway. >>> access_ok is _only_ valid in combination with copy_from/to_user and >>> friends and is an optimization of the access checks depending upon >>> architecture. You always need them both, one alone is useless. >> ENOPARSE, access_ok will always return the same value for a given address at >> least >> on x86 so if address supplied at ioctl time is a valid userspace address >> then it >> will still be a valid userspace address at buffer object bind time (note >> that the >> user address is immutable here). So access_ok can be done once and only once >> inside >> the ioctl and then for the write permission you need to recheck the vma each >> time >> you bind the object (or rather each time the previous bind was invalidated >> by some >> mmu_notifier call). >> >> That being said access_ok is kind of useless given that get_user_page will >> fail on >> kernel address and i assume for any special address any architecture might >> have. So >> strictly speaking the access_ok is just a way to fail early and flatout >> instead of >> delaying the failure to bind time. > Well that's what I've tried to say. For gup you don't need access_ok, > that's really just one part of copy_from/to_user machinery. And afaik it's > not specified what exactly access_ok checks (on x86 it only checks for the > kernel address limit) so I don't think there's a lot of use in it for gup. > > Maybe I should have done an s/valid/useful/ in my short comment. I've dropped the access_ok check and also fixed another bug in the VM handling. Patches are on their way to the list. Thanks for the comments, Christian. > -Daniel
