From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 2/5] drm/radeon: add userptr flag to limit it to anonymous memory v2 Date: Thu, 7 Aug 2014 08:55:39 +0200 Message-ID: <20140807065539.GA8727@phenom.ffwll.local> References: <1407254732-8332-2-git-send-email-deathsimple@vodafone.de> <20140805173932.GA2854@gmail.com> <53E11831.1010809@vodafone.de> <20140805221334.GA3991@gmail.com> <53E1D160.4040804@vodafone.de> <20140806160847.GA3142@gmail.com> <53E26325.8070309@vodafone.de> <20140806183415.GA4716@gmail.com> <20140806202431.GW8727@phenom.ffwll.local> <20140807034547.GA7046@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-we0-f174.google.com (mail-we0-f174.google.com [74.125.82.174]) by gabe.freedesktop.org (Postfix) with ESMTP id 1627D6E2EF for ; Wed, 6 Aug 2014 23:55:27 -0700 (PDT) Received: by mail-we0-f174.google.com with SMTP id x48so3653408wes.5 for ; Wed, 06 Aug 2014 23:55:27 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140807034547.GA7046@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Jerome Glisse Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org 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=F6nig wrote: > > > > Am 06.08.2014 um 18:08 schrieb Jerome Glisse: > > > > >On Wed, Aug 06, 2014 at 08:55:28AM +0200, Christian K=F6nig wrote: > > > > >>Am 06.08.2014 um 00:13 schrieb Jerome Glisse: > > > > >>>On Tue, Aug 05, 2014 at 07:45:21PM +0200, Christian K=F6nig wrot= e: > > > > >>>>Am 05.08.2014 um 19:39 schrieb Jerome Glisse: > > > > >>>>>On Tue, Aug 05, 2014 at 06:05:29PM +0200, Christian K=F6nig wr= ote: > > > > >>>>>>From: Christian K=F6nig > > > > >>>>>> > > > > >>>>>>Avoid problems with writeback by limiting userptr to anonymou= s memory. > > > > >>>>>> > > > > >>>>>>v2: add commit and code comments > > > > >>>>>I guess, i have not expressed myself clearly. This is bogus, y= ou pretend > > > > >>>>>you want to avoid writeback issue but you still allow userspac= e to map > > > > >>>>>file backed pages (which by the way might be a regular bo obje= ct from > > > > >>>>>another device for instance and that would be fun). > > > > >>>>> > > > > >>>>>So this patch is a no go and i would rather see that this user= ptr 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 impl= ement write > > > > >>>is broken. Here is how it can abuse to allow write to a file bac= ked mmap. > > > > >>> > > > > >>>mmap(fixaddress,fixedsize,NOFD) > > > > >>>userptr_ioctl(fixedaddress, RADEON_GEM_USERPTR_ANONONLY) > > > > >>>// bo is created successfully because fixedaddress is part of an= onvma > > > > >>>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_wr= ite_to) > > > > >>>radeon_ioctl_use_my_userptrbo > > > > >>>// bo is bind again by radeon and because all flag are set at cr= eation > > > > >>>// 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 remov= e 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 com= mand > > > > >>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 an= d i am > > > pretty sure on x86 the VERIFY_WRITE or VERIFY_READ is just flat out i= gnored. > > > = > > > 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 o= nce inside > the ioctl and then for the write permission you need to recheck the vma e= ach time > you bind the object (or rather each time the previous bind was invalidate= d by some > mmu_notifier call). > = > That being said access_ok is kind of useless given that get_user_page wil= l fail on > kernel address and i assume for any special address any architecture migh= t have. So > strictly speaking the access_ok is just a way to fail early and flatout i= nstead of > delaying the failure to bind time. Well that's what I've tried to say. For gup you don't need access_ok, that's really just one part of copy_from/to_user machinery. And afaik it's not specified what exactly access_ok checks (on x86 it only checks for the kernel address limit) so I don't think there's a lot of use in it for gup. Maybe I should have done an s/valid/useful/ in my short comment. -Daniel -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch