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

["Christian König" <deathsimple@vodafone.de>]

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@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