[PATCH 2/5] drm/radeon: add userptr flag to limit it to anonymous memory v2

[Christian König]

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