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

[Jerome Glisse <j.glisse@gmail.com>]

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.

Cheers,
Jérôme

> Christian.
> 
> >
> >Cheers,
> >Jérôme
> >
> >>Signed-off-by: Christian König <christian.koenig@amd.com>
> >>---
> >>  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@lists.freedesktop.org
> >>http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel