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

[Jerome Glisse]

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.
> 
> Cheers,
> Jérôme

I should add that access_ok should be done only once inside the gem userptr ioctl
as access_ok only check that the address is a valid userspace address and not some
special address (like an address inside the kernel address space or a non canonical
address on x86-64 ...). It it returns true the first time then it will always return
true.

Only vma need to be checked on each bind.

> 
> > 
> > Christian.
> > 
> > >
> > >Cheers,
> > >Jérôme
> >