From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jerome Glisse Subject: Re: [PATCH 2/5] drm/radeon: add userptr flag to limit it to anonymous memory v2 Date: Tue, 5 Aug 2014 18:13:35 -0400 Message-ID: <20140805221334.GA3991@gmail.com> References: <1407254732-8332-1-git-send-email-deathsimple@vodafone.de> <1407254732-8332-2-git-send-email-deathsimple@vodafone.de> <20140805173932.GA2854@gmail.com> <53E11831.1010809@vodafone.de> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-qg0-f47.google.com (mail-qg0-f47.google.com [209.85.192.47]) by gabe.freedesktop.org (Postfix) with ESMTP id D0A056E529 for ; Tue, 5 Aug 2014 15:13:42 -0700 (PDT) Received: by mail-qg0-f47.google.com with SMTP id i50so1782301qgf.20 for ; Tue, 05 Aug 2014 15:13:42 -0700 (PDT) Content-Disposition: inline In-Reply-To: <53E11831.1010809@vodafone.de> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Christian =?iso-8859-1?Q?K=F6nig?= Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Tue, Aug 05, 2014 at 07:45:21PM +0200, Christian K=F6nig wrote: > Am 05.08.2014 um 19:39 schrieb Jerome Glisse: > >On Tue, Aug 05, 2014 at 06:05:29PM +0200, Christian K=F6nig wrote: > >>From: Christian K=F6nig > >> > >>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=E9r=F4me > Christian. > = > > > >Cheers, > >J=E9r=F4me > > > >>Signed-off-by: Christian K=F6nig > >>--- > >> 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/rade= on/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/rade= on/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 =3D gtt->userptr + ttm->num_pages * PAGE_SIZE; > >>+ struct vm_area_struct *vma; > >>+ vma =3D find_vma(gtt->usermm, gtt->userptr); > >>+ if (!vma || vma->vm_file || vma->vm_end < end) > >>+ return -EPERM; > >>+ } > >>+ > >> do { > >> unsigned num_pages =3D ttm->num_pages - pinned; > >> uint64_t userptr =3D gtt->userptr + pinned * PAGE_SIZE; > >>diff --git a/include/uapi/drm/radeon_drm.h b/include/uapi/drm/radeon_dr= m.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