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@xxxxxxx>
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 <christian.koenig@xxxxxxx>
---
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 addr;
--
1.9.1
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
