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

2014-08-07 Thread 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 
>>>
>>> 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

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

2014-08-07 Thread Christian König 
From: Christian K?nig 

Avoid problems with writeback by limiting userptr to anonymous memory.

v2: add commit and code comments

Signed-off-by: Christian K?nig 
---
 drivers/gpu/drm/radeon/radeon_gem.c |  3 ++-
 drivers/gpu/drm/radeon/radeon_ttm.c | 10 ++
 include/uapi/drm/radeon_drm.h   |  1 +
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_gem.c 
b/drivers/gpu/drm/radeon/radeon_gem.c
index 993ab22..032736b 100644
--- a/drivers/gpu/drm/radeon/radeon_gem.c
+++ b/drivers/gpu/drm/radeon/radeon_gem.c
@@ -290,7 +290,8 @@ int radeon_gem_userptr_ioctl(struct drm_device *dev, void 
*data,
return -EACCES;

/* reject unknown flag values */
-   if (args->flags & ~RADEON_GEM_USERPTR_READONLY)
+   if (args->flags & ~(RADEON_GEM_USERPTR_READONLY |
+   RADEON_GEM_USERPTR_ANONONLY))
return -EINVAL;

/* readonly pages not tested on older hardware */
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c 
b/drivers/gpu/drm/radeon/radeon_ttm.c
index b20933f..12e37b1 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -538,6 +538,16 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_tt *ttm)
if (current->mm != gtt->usermm)
return -EPERM;

+   if (gtt->userflags & RADEON_GEM_USERPTR_ANONONLY) {
+   /* check that we only pin down anonymous memory
+  to prevent problems with writeback */
+   unsigned long end = gtt->userptr + ttm->num_pages * PAGE_SIZE;
+   struct vm_area_struct *vma;
+   vma = find_vma(gtt->usermm, gtt->userptr);
+   if (!vma || vma->vm_file || vma->vm_end < end)
+   return -EPERM;
+   }
+
do {
unsigned num_pages = ttm->num_pages - pinned;
uint64_t userptr = gtt->userptr + pinned * PAGE_SIZE;
diff --git a/include/uapi/drm/radeon_drm.h b/include/uapi/drm/radeon_drm.h
index 3a9f209..9720e1a 100644
--- a/include/uapi/drm/radeon_drm.h
+++ b/include/uapi/drm/radeon_drm.h
@@ -816,6 +816,7 @@ struct drm_radeon_gem_create {
  * perform any operation.
  */
 #define RADEON_GEM_USERPTR_READONLY(1 << 0)
+#define RADEON_GEM_USERPTR_ANONONLY(1 << 1)

 struct drm_radeon_gem_userptr {
uint64_taddr;
-- 
1.9.1

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

2014-08-07 Thread 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 
> > > > >>
> > > > >>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.
> > 
> > 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
>

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

2014-08-07 Thread Jerome Glisse 
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 
> > > >>
> > > >>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.
> 
> 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

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

2014-08-06 Thread Daniel Vetter 
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 
> > >>
> > >>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.

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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

2014-08-06 Thread Christian König 
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 
>>
>> 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

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

2014-08-06 Thread 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 
> > >>
> > >>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
> >

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

2014-08-06 Thread Jerome Glisse 
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 
> >>
> >>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

> 
> Christian.
> 
> >
> >Cheers,
> >J?r?me
>

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

2014-08-06 Thread 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 
> 
> 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.

Cheers,
J?r?me

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

2014-08-06 Thread Christian König 
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 

 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.

Regards,
Christian.

>
> Cheers,
> J?r?me
>
>> Christian.
>>
>>> Cheers,
>>> J?r?me
>>>
 Signed-off-by: Christian K?nig 
 ---
   drivers/gpu/drm/radeon/radeon_gem.c |  3 ++-
   drivers/gpu/drm/radeon/radeon_ttm.c | 10 ++
   include/uapi/drm/radeon_drm.h   |  1 +
   3 files changed, 13 insertions(+), 1 deletion(-)

 diff --git a/drivers/gpu/drm/radeon/radeon_gem.c 
 b/drivers/gpu/drm/radeon/radeon_gem.c
 index 993ab22..032736b 100644
 --- a/drivers/gpu/drm/radeon/radeon_gem.c
 +++ b/drivers/gpu/drm/radeon/radeon_gem.c
 @@ -290,7 +290,8 @@ int radeon_gem_userptr_ioctl(struct drm_device *dev, 
 void *data,
return -EACCES;
/* reject unknown flag values */
 -  if (args->flags & ~RADEON_GEM_USERPTR_READONLY)
 +  if (args->flags & ~(RADEON_GEM_USERPTR_READONLY |
 +  RADEON_GEM_USERPTR_ANONONLY))
return -EINVAL;
/* readonly pages not tested on older hardware */
 diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c 
 b/drivers/gpu/drm/radeon/radeon_ttm.c
 index 0109090..54eb7bc 100644
 --- a/drivers/gpu/drm/radeon/radeon_ttm.c
 +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
 @@ -542,6 +542,16 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_tt 
 *ttm)
   ttm->num_pages * PAGE_SIZE))
return -EFAULT;
 +  if (gtt->userflags & RADEON_GEM_USERPTR_ANONONLY) {
 +  /* check that we only pin down anonymous memory
 + to prevent problems with writeback */
 +  unsigned long end = gtt->userptr + ttm->num_pages * PAGE_SIZE;
 +  struct vm_area_struct *vma;
 +  vma = find_vma(gtt->usermm, gtt->userptr);
 +  if (!vma || vma->vm_file || vma->vm_end < end)
 +  return -EPERM;
 +  }
 +
do {
unsigned num_pages = ttm->num_pages - pinned;
uint64_t userptr = gtt->userptr + pinned * PAGE_SIZE;
 diff --git a/include/uapi/drm/radeon_drm.h b/include/uapi/drm/radeon_drm.h
 index 3a9f209..9720e1a 100644
 --- a/include/uapi/drm/radeon_drm.h
 +++ b/include/uapi/drm/radeon_drm.h
 @@ -816,6 +816,7 @@ struct drm_radeon_gem_create {
* perform any operation.
*/
   #define RADEON_GEM_USERPTR_READONLY  (1 << 0)
 +#define RADEON_GEM_USERPTR_ANONONLY   (1 << 1)
   struct drm_radeon_gem_userptr {
uint64_t

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

2014-08-05 Thread Christian König 
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 
>>
>> 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.

Christian.

>
> Cheers,
> J?r?me
>
>> Signed-off-by: Christian K?nig 
>> ---
>>   drivers/gpu/drm/radeon/radeon_gem.c |  3 ++-
>>   drivers/gpu/drm/radeon/radeon_ttm.c | 10 ++
>>   include/uapi/drm/radeon_drm.h   |  1 +
>>   3 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon_gem.c 
>> b/drivers/gpu/drm/radeon/radeon_gem.c
>> index 993ab22..032736b 100644
>> --- a/drivers/gpu/drm/radeon/radeon_gem.c
>> +++ b/drivers/gpu/drm/radeon/radeon_gem.c
>> @@ -290,7 +290,8 @@ int radeon_gem_userptr_ioctl(struct drm_device *dev, 
>> void *data,
>>  return -EACCES;
>>   
>>  /* reject unknown flag values */
>> -if (args->flags & ~RADEON_GEM_USERPTR_READONLY)
>> +if (args->flags & ~(RADEON_GEM_USERPTR_READONLY |
>> +RADEON_GEM_USERPTR_ANONONLY))
>>  return -EINVAL;
>>   
>>  /* readonly pages not tested on older hardware */
>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c 
>> b/drivers/gpu/drm/radeon/radeon_ttm.c
>> index 0109090..54eb7bc 100644
>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
>> @@ -542,6 +542,16 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_tt *ttm)
>> ttm->num_pages * PAGE_SIZE))
>>  return -EFAULT;
>>   
>> +if (gtt->userflags & RADEON_GEM_USERPTR_ANONONLY) {
>> +/* check that we only pin down anonymous memory
>> +   to prevent problems with writeback */
>> +unsigned long end = gtt->userptr + ttm->num_pages * PAGE_SIZE;
>> +struct vm_area_struct *vma;
>> +vma = find_vma(gtt->usermm, gtt->userptr);
>> +if (!vma || vma->vm_file || vma->vm_end < end)
>> +return -EPERM;
>> +}
>> +
>>  do {
>>  unsigned num_pages = ttm->num_pages - pinned;
>>  uint64_t userptr = gtt->userptr + pinned * PAGE_SIZE;
>> diff --git a/include/uapi/drm/radeon_drm.h b/include/uapi/drm/radeon_drm.h
>> index 3a9f209..9720e1a 100644
>> --- a/include/uapi/drm/radeon_drm.h
>> +++ b/include/uapi/drm/radeon_drm.h
>> @@ -816,6 +816,7 @@ struct drm_radeon_gem_create {
>>* perform any operation.
>>*/
>>   #define RADEON_GEM_USERPTR_READONLY(1 << 0)
>> +#define RADEON_GEM_USERPTR_ANONONLY (1 << 1)
>>   
>>   struct drm_radeon_gem_userptr {
>>  uint64_taddr;
>> -- 
>> 1.9.1
>>
>> ___
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2014-08-05 Thread 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 
> >>
> >>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.

Cheers,
J?r?me

> Christian.
> 
> >
> >Cheers,
> >J?r?me
> >
> >>Signed-off-by: Christian K?nig 
> >>---
> >>  drivers/gpu/drm/radeon/radeon_gem.c |  3 ++-
> >>  drivers/gpu/drm/radeon/radeon_ttm.c | 10 ++
> >>  include/uapi/drm/radeon_drm.h   |  1 +
> >>  3 files changed, 13 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/gpu/drm/radeon/radeon_gem.c 
> >>b/drivers/gpu/drm/radeon/radeon_gem.c
> >>index 993ab22..032736b 100644
> >>--- a/drivers/gpu/drm/radeon/radeon_gem.c
> >>+++ b/drivers/gpu/drm/radeon/radeon_gem.c
> >>@@ -290,7 +290,8 @@ int radeon_gem_userptr_ioctl(struct drm_device *dev, 
> >>void *data,
> >>return -EACCES;
> >>/* reject unknown flag values */
> >>-   if (args->flags & ~RADEON_GEM_USERPTR_READONLY)
> >>+   if (args->flags & ~(RADEON_GEM_USERPTR_READONLY |
> >>+   RADEON_GEM_USERPTR_ANONONLY))
> >>return -EINVAL;
> >>/* readonly pages not tested on older hardware */
> >>diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c 
> >>b/drivers/gpu/drm/radeon/radeon_ttm.c
> >>index 0109090..54eb7bc 100644
> >>--- a/drivers/gpu/drm/radeon/radeon_ttm.c
> >>+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> >>@@ -542,6 +542,16 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_tt 
> >>*ttm)
> >>   ttm->num_pages * PAGE_SIZE))
> >>return -EFAULT;
> >>+   if (gtt->userflags & RADEON_GEM_USERPTR_ANONONLY) {
> >>+   /* check that we only pin down anonymous memory
> >>+  to prevent problems with writeback */
> >>+   unsigned long end = gtt->userptr + ttm->num_pages * PAGE_SIZE;
> >>+   struct vm_area_struct *vma;
> >>+   vma = find_vma(gtt->usermm, gtt->userptr);
> >>+   if (!vma || vma->vm_file || vma->vm_end < end)
> >>+   return -EPERM;
> >>+   }
> >>+
> >>do {
> >>unsigned num_pages = ttm->num_pages - pinned;
> >>uint64_t userptr = gtt->userptr + pinned * PAGE_SIZE;
> >>diff --git a/include/uapi/drm/radeon_drm.h b/include/uapi/drm/radeon_drm.h
> >>index 3a9f209..9720e1a 100644
> >>--- a/include/uapi/drm/radeon_drm.h
> >>+++ b/include/uapi/drm/radeon_drm.h
> >>@@ -816,6 +816,7 @@ struct drm_radeon_gem_create {
> >>   * perform any operation.
> >>   */
> >>  #define RADEON_GEM_USERPTR_READONLY   (1 << 0)
> >>+#define RADEON_GEM_USERPTR_ANONONLY(1 << 1)
> >>  struct drm_radeon_gem_userptr {
> >>uint64_taddr;
> >>-- 
> >>1.9.1
> >>
> >>___
> >>dri-devel mailing list
> >>dri-devel at lists.freedesktop.org
> >>http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2014-08-05 Thread Christian König 
From: Christian K?nig 

Avoid problems with writeback by limiting userptr to anonymous memory.

v2: add commit and code comments

Signed-off-by: Christian K?nig 
---
 drivers/gpu/drm/radeon/radeon_gem.c |  3 ++-
 drivers/gpu/drm/radeon/radeon_ttm.c | 10 ++
 include/uapi/drm/radeon_drm.h   |  1 +
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_gem.c 
b/drivers/gpu/drm/radeon/radeon_gem.c
index 993ab22..032736b 100644
--- a/drivers/gpu/drm/radeon/radeon_gem.c
+++ b/drivers/gpu/drm/radeon/radeon_gem.c
@@ -290,7 +290,8 @@ int radeon_gem_userptr_ioctl(struct drm_device *dev, void 
*data,
return -EACCES;

/* reject unknown flag values */
-   if (args->flags & ~RADEON_GEM_USERPTR_READONLY)
+   if (args->flags & ~(RADEON_GEM_USERPTR_READONLY |
+   RADEON_GEM_USERPTR_ANONONLY))
return -EINVAL;

/* readonly pages not tested on older hardware */
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c 
b/drivers/gpu/drm/radeon/radeon_ttm.c
index 0109090..54eb7bc 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -542,6 +542,16 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_tt *ttm)
   ttm->num_pages * PAGE_SIZE))
return -EFAULT;

+   if (gtt->userflags & RADEON_GEM_USERPTR_ANONONLY) {
+   /* check that we only pin down anonymous memory
+  to prevent problems with writeback */
+   unsigned long end = gtt->userptr + ttm->num_pages * PAGE_SIZE;
+   struct vm_area_struct *vma;
+   vma = find_vma(gtt->usermm, gtt->userptr);
+   if (!vma || vma->vm_file || vma->vm_end < end)
+   return -EPERM;
+   }
+
do {
unsigned num_pages = ttm->num_pages - pinned;
uint64_t userptr = gtt->userptr + pinned * PAGE_SIZE;
diff --git a/include/uapi/drm/radeon_drm.h b/include/uapi/drm/radeon_drm.h
index 3a9f209..9720e1a 100644
--- a/include/uapi/drm/radeon_drm.h
+++ b/include/uapi/drm/radeon_drm.h
@@ -816,6 +816,7 @@ struct drm_radeon_gem_create {
  * perform any operation.
  */
 #define RADEON_GEM_USERPTR_READONLY(1 << 0)
+#define RADEON_GEM_USERPTR_ANONONLY(1 << 1)

 struct drm_radeon_gem_userptr {
uint64_taddr;
-- 
1.9.1

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

2014-08-05 Thread Christian König 
Am 05.08.2014 um 16:24 schrieb Jerome Glisse:
> On Tue, Aug 05, 2014 at 04:11:04PM +0200, Christian K?nig wrote:
>> From: Christian K?nig 
> Why do you want that ?
To avoid any problems with writeback (which as far as I understand 
should only happen on mmaped files).

> NACK until proper explanation and motive.
Going to update the commit message and add a code comment.

Christian.

>
>> Signed-off-by: Christian K?nig 
>> ---
>>   drivers/gpu/drm/radeon/radeon_gem.c | 3 ++-
>>   drivers/gpu/drm/radeon/radeon_ttm.c | 8 
>>   include/uapi/drm/radeon_drm.h   | 3 ++-
>>   3 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon_gem.c 
>> b/drivers/gpu/drm/radeon/radeon_gem.c
>> index 993ab22..032736b 100644
>> --- a/drivers/gpu/drm/radeon/radeon_gem.c
>> +++ b/drivers/gpu/drm/radeon/radeon_gem.c
>> @@ -290,7 +290,8 @@ int radeon_gem_userptr_ioctl(struct drm_device *dev, 
>> void *data,
>>  return -EACCES;
>>   
>>  /* reject unknown flag values */
>> -if (args->flags & ~RADEON_GEM_USERPTR_READONLY)
>> +if (args->flags & ~(RADEON_GEM_USERPTR_READONLY |
>> +RADEON_GEM_USERPTR_ANONONLY))
>>  return -EINVAL;
>>   
>>  /* readonly pages not tested on older hardware */
>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c 
>> b/drivers/gpu/drm/radeon/radeon_ttm.c
>> index 0109090..d63e698 100644
>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
>> @@ -542,6 +542,14 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_tt *ttm)
>> ttm->num_pages * PAGE_SIZE))
>>  return -EFAULT;
>>   
>> +if (gtt->userflags & RADEON_GEM_USERPTR_ANONONLY) {
>> +unsigned long end = gtt->userptr + ttm->num_pages * PAGE_SIZE;
>> +struct vm_area_struct *vma;
>> +vma = find_vma(gtt->usermm, gtt->userptr);
>> +if (!vma || vma->vm_file || vma->vm_end < end)
>> +return -EPERM;
>> +}
>> +
>>  do {
>>  unsigned num_pages = ttm->num_pages - pinned;
>>  uint64_t userptr = gtt->userptr + pinned * PAGE_SIZE;
>> diff --git a/include/uapi/drm/radeon_drm.h b/include/uapi/drm/radeon_drm.h
>> index a18ec54..4080ad3 100644
>> --- a/include/uapi/drm/radeon_drm.h
>> +++ b/include/uapi/drm/radeon_drm.h
>> @@ -810,7 +810,8 @@ struct drm_radeon_gem_create {
>>  uint32_tflags;
>>   };
>>   
>> -#define RADEON_GEM_USERPTR_READONLY 0x1
>> +#define RADEON_GEM_USERPTR_READONLY (1 << 0)
>> +#define RADEON_GEM_USERPTR_ANONONLY (1 << 1)
>>   
>>   struct drm_radeon_gem_userptr {
>>  uint64_taddr;
>> -- 
>> 1.9.1
>>
>> ___
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2014-08-05 Thread Christian König 
From: Christian K?nig 

Signed-off-by: Christian K?nig 
---
 drivers/gpu/drm/radeon/radeon_gem.c | 3 ++-
 drivers/gpu/drm/radeon/radeon_ttm.c | 8 
 include/uapi/drm/radeon_drm.h   | 3 ++-
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_gem.c 
b/drivers/gpu/drm/radeon/radeon_gem.c
index 993ab22..032736b 100644
--- a/drivers/gpu/drm/radeon/radeon_gem.c
+++ b/drivers/gpu/drm/radeon/radeon_gem.c
@@ -290,7 +290,8 @@ int radeon_gem_userptr_ioctl(struct drm_device *dev, void 
*data,
return -EACCES;

/* reject unknown flag values */
-   if (args->flags & ~RADEON_GEM_USERPTR_READONLY)
+   if (args->flags & ~(RADEON_GEM_USERPTR_READONLY |
+   RADEON_GEM_USERPTR_ANONONLY))
return -EINVAL;

/* readonly pages not tested on older hardware */
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c 
b/drivers/gpu/drm/radeon/radeon_ttm.c
index 0109090..d63e698 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -542,6 +542,14 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_tt *ttm)
   ttm->num_pages * PAGE_SIZE))
return -EFAULT;

+   if (gtt->userflags & RADEON_GEM_USERPTR_ANONONLY) {
+   unsigned long end = gtt->userptr + ttm->num_pages * PAGE_SIZE;
+   struct vm_area_struct *vma;
+   vma = find_vma(gtt->usermm, gtt->userptr);
+   if (!vma || vma->vm_file || vma->vm_end < end)
+   return -EPERM;
+   }
+
do {
unsigned num_pages = ttm->num_pages - pinned;
uint64_t userptr = gtt->userptr + pinned * PAGE_SIZE;
diff --git a/include/uapi/drm/radeon_drm.h b/include/uapi/drm/radeon_drm.h
index a18ec54..4080ad3 100644
--- a/include/uapi/drm/radeon_drm.h
+++ b/include/uapi/drm/radeon_drm.h
@@ -810,7 +810,8 @@ struct drm_radeon_gem_create {
uint32_tflags;
 };

-#define RADEON_GEM_USERPTR_READONLY0x1
+#define RADEON_GEM_USERPTR_READONLY(1 << 0)
+#define RADEON_GEM_USERPTR_ANONONLY(1 << 1)

 struct drm_radeon_gem_userptr {
uint64_taddr;
-- 
1.9.1

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

2014-08-05 Thread Jerome Glisse 
On Tue, Aug 05, 2014 at 06:05:29PM +0200, Christian K?nig wrote:
> From: Christian K?nig 
> 
> 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.

Cheers,
J?r?me

> 
> Signed-off-by: Christian K?nig 
> ---
>  drivers/gpu/drm/radeon/radeon_gem.c |  3 ++-
>  drivers/gpu/drm/radeon/radeon_ttm.c | 10 ++
>  include/uapi/drm/radeon_drm.h   |  1 +
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/radeon/radeon_gem.c 
> b/drivers/gpu/drm/radeon/radeon_gem.c
> index 993ab22..032736b 100644
> --- a/drivers/gpu/drm/radeon/radeon_gem.c
> +++ b/drivers/gpu/drm/radeon/radeon_gem.c
> @@ -290,7 +290,8 @@ int radeon_gem_userptr_ioctl(struct drm_device *dev, void 
> *data,
>   return -EACCES;
>  
>   /* reject unknown flag values */
> - if (args->flags & ~RADEON_GEM_USERPTR_READONLY)
> + if (args->flags & ~(RADEON_GEM_USERPTR_READONLY |
> + RADEON_GEM_USERPTR_ANONONLY))
>   return -EINVAL;
>  
>   /* readonly pages not tested on older hardware */
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c 
> b/drivers/gpu/drm/radeon/radeon_ttm.c
> index 0109090..54eb7bc 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -542,6 +542,16 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_tt *ttm)
>  ttm->num_pages * PAGE_SIZE))
>   return -EFAULT;
>  
> + if (gtt->userflags & RADEON_GEM_USERPTR_ANONONLY) {
> + /* check that we only pin down anonymous memory
> +to prevent problems with writeback */
> + unsigned long end = gtt->userptr + ttm->num_pages * PAGE_SIZE;
> + struct vm_area_struct *vma;
> + vma = find_vma(gtt->usermm, gtt->userptr);
> + if (!vma || vma->vm_file || vma->vm_end < end)
> + return -EPERM;
> + }
> +
>   do {
>   unsigned num_pages = ttm->num_pages - pinned;
>   uint64_t userptr = gtt->userptr + pinned * PAGE_SIZE;
> diff --git a/include/uapi/drm/radeon_drm.h b/include/uapi/drm/radeon_drm.h
> index 3a9f209..9720e1a 100644
> --- a/include/uapi/drm/radeon_drm.h
> +++ b/include/uapi/drm/radeon_drm.h
> @@ -816,6 +816,7 @@ struct drm_radeon_gem_create {
>   * perform any operation.
>   */
>  #define RADEON_GEM_USERPTR_READONLY  (1 << 0)
> +#define RADEON_GEM_USERPTR_ANONONLY  (1 << 1)
>  
>  struct drm_radeon_gem_userptr {
>   uint64_taddr;
> -- 
> 1.9.1
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2014-08-05 Thread Jerome Glisse 
On Tue, Aug 05, 2014 at 04:11:04PM +0200, Christian K?nig wrote:
> From: Christian K?nig 

Why do you want that ?

NACK until proper explanation and motive.

> 
> Signed-off-by: Christian K?nig 
> ---
>  drivers/gpu/drm/radeon/radeon_gem.c | 3 ++-
>  drivers/gpu/drm/radeon/radeon_ttm.c | 8 
>  include/uapi/drm/radeon_drm.h   | 3 ++-
>  3 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/radeon/radeon_gem.c 
> b/drivers/gpu/drm/radeon/radeon_gem.c
> index 993ab22..032736b 100644
> --- a/drivers/gpu/drm/radeon/radeon_gem.c
> +++ b/drivers/gpu/drm/radeon/radeon_gem.c
> @@ -290,7 +290,8 @@ int radeon_gem_userptr_ioctl(struct drm_device *dev, void 
> *data,
>   return -EACCES;
>  
>   /* reject unknown flag values */
> - if (args->flags & ~RADEON_GEM_USERPTR_READONLY)
> + if (args->flags & ~(RADEON_GEM_USERPTR_READONLY |
> + RADEON_GEM_USERPTR_ANONONLY))
>   return -EINVAL;
>  
>   /* readonly pages not tested on older hardware */
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c 
> b/drivers/gpu/drm/radeon/radeon_ttm.c
> index 0109090..d63e698 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -542,6 +542,14 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_tt *ttm)
>  ttm->num_pages * PAGE_SIZE))
>   return -EFAULT;
>  
> + if (gtt->userflags & RADEON_GEM_USERPTR_ANONONLY) {
> + unsigned long end = gtt->userptr + ttm->num_pages * PAGE_SIZE;
> + struct vm_area_struct *vma;
> + vma = find_vma(gtt->usermm, gtt->userptr);
> + if (!vma || vma->vm_file || vma->vm_end < end)
> + return -EPERM;
> + }
> +
>   do {
>   unsigned num_pages = ttm->num_pages - pinned;
>   uint64_t userptr = gtt->userptr + pinned * PAGE_SIZE;
> diff --git a/include/uapi/drm/radeon_drm.h b/include/uapi/drm/radeon_drm.h
> index a18ec54..4080ad3 100644
> --- a/include/uapi/drm/radeon_drm.h
> +++ b/include/uapi/drm/radeon_drm.h
> @@ -810,7 +810,8 @@ struct drm_radeon_gem_create {
>   uint32_tflags;
>  };
>  
> -#define RADEON_GEM_USERPTR_READONLY  0x1
> +#define RADEON_GEM_USERPTR_READONLY  (1 << 0)
> +#define RADEON_GEM_USERPTR_ANONONLY  (1 << 1)
>  
>  struct drm_radeon_gem_userptr {
>   uint64_taddr;
> -- 
> 1.9.1
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2014-07-28 Thread Christian König 
From: Christian K?nig 

Signed-off-by: Christian K?nig 
---
 drivers/gpu/drm/radeon/radeon_gem.c | 3 ++-
 drivers/gpu/drm/radeon/radeon_ttm.c | 8 
 include/uapi/drm/radeon_drm.h   | 3 ++-
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_gem.c 
b/drivers/gpu/drm/radeon/radeon_gem.c
index 05b9661..e2c6f44 100644
--- a/drivers/gpu/drm/radeon/radeon_gem.c
+++ b/drivers/gpu/drm/radeon/radeon_gem.c
@@ -292,7 +292,8 @@ int radeon_gem_userptr_ioctl(struct drm_device *dev, void 
*data,
return -EACCES;

/* reject unknown flag values */
-   if (args->flags & ~RADEON_GEM_USERPTR_READONLY)
+   if (args->flags & ~(RADEON_GEM_USERPTR_READONLY |
+   RADEON_GEM_USERPTR_ANONONLY))
return -EINVAL;

/* readonly pages not tested on older hardware */
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c 
b/drivers/gpu/drm/radeon/radeon_ttm.c
index dbc15ec..7a21478 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -541,6 +541,14 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_tt *ttm)
   ttm->num_pages * PAGE_SIZE))
return -EFAULT;

+   if (gtt->userflags & RADEON_GEM_USERPTR_ANONONLY) {
+   unsigned long end = gtt->userptr + ttm->num_pages * PAGE_SIZE;
+   struct vm_area_struct *vma;
+   vma = find_vma(gtt->usermm, gtt->userptr);
+   if (!vma || vma->vm_file || vma->vm_end < end)
+   return -EPERM;
+   }
+
do {
unsigned num_pages = ttm->num_pages - pinned;
uint64_t userptr = gtt->userptr + pinned * PAGE_SIZE;
diff --git a/include/uapi/drm/radeon_drm.h b/include/uapi/drm/radeon_drm.h
index a18ec54..4080ad3 100644
--- a/include/uapi/drm/radeon_drm.h
+++ b/include/uapi/drm/radeon_drm.h
@@ -810,7 +810,8 @@ struct drm_radeon_gem_create {
uint32_tflags;
 };

-#define RADEON_GEM_USERPTR_READONLY0x1
+#define RADEON_GEM_USERPTR_READONLY(1 << 0)
+#define RADEON_GEM_USERPTR_ANONONLY(1 << 1)

 struct drm_radeon_gem_userptr {
uint64_taddr;
-- 
1.9.1