From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?windows-1252?Q?Christian_K=F6nig?= Subject: Re: [PATCH 2/5] drm/radeon: add userptr flag to limit it to anonymous memory v2 Date: Thu, 07 Aug 2014 09:36:11 +0200 Message-ID: <53E32C6B.2090208@vodafone.de> 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> <20140807065539.GA8727@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from pegasos-out.vodafone.de (pegasos-out.vodafone.de [80.84.1.38]) by gabe.freedesktop.org (Postfix) with ESMTP id F15866E72F for ; Thu, 7 Aug 2014 00:36:53 -0700 (PDT) Received: from localhost (localhost.localdomain [127.0.0.1]) by pegasos-out.vodafone.de (Rohrpostix1 Daemon) with ESMTP id 6BE682613ED for ; Thu, 7 Aug 2014 09:36:53 +0200 (CEST) Received: from pegasos-out.vodafone.de ([127.0.0.1]) by localhost (rohrpostix1.prod.vfnet.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id HUROlkrp6i6P for ; Thu, 7 Aug 2014 09:36:31 +0200 (CEST) In-Reply-To: <20140807065539.GA8727@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter , Jerome Glisse Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org 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=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 wrote: >>>>>>>>> Am 05.08.2014 um 19:39 schrieb Jerome Glisse: >>>>>>>>>> On Tue, Aug 05, 2014 at 06:05:29PM +0200, Christian K=F6nig wrot= e: >>>>>>>>>>> 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 userpt= r to >>>>>>>>>> be restricted to anon vma only no matter what. No flags here. >>>>>>>>> Mapping of non anonymous memory (e.g. everything get_user_pages w= on'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 yo= u say it's >>>>>>>>> a must have. >>>>>>>>> >>>>>>>> Well for time being you should force read only. The way you implem= ent write >>>>>>>> is broken. Here is how it can abuse to allow write to a file backe= d mmap. >>>>>>>> >>>>>>>> mmap(fixaddress,fixedsize,NOFD) >>>>>>>> userptr_ioctl(fixedaddress, RADEON_GEM_USERPTR_ANONONLY) >>>>>>>> // bo is created successfully because fixedaddress is part of anon= vma >>>>>>>> munmap(fixedaddress,fixedsize) >>>>>>>> // radeon get mmu_notifier_range_start callback and unbind page fr= om the >>>>>>>> // bo but radeon does not know there was an unmap. >>>>>>>> mmap(fixaddress,fixedsize,fd_to_this_read_only_file_i_want_to_writ= e_to) >>>>>>>> radeon_ioctl_use_my_userptrbo >>>>>>>> // bo is bind again by radeon and because all flag are set at crea= tion >>>>>>>> // 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 th= is >>>>>>>> case somehow. >>>>>>> I've stumbled over this attack vector as well and it's the reason w= hy 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 comma= nd >>>>>>> submission time. >>>>>> So you seem immune to that issue but you are still not checking if t= he 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 ig= nored. >>>> 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 addres= s 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 invalidat= ed by some >> mmu_notifier call). >> >> That being said access_ok is kind of useless given that get_user_page wi= ll fail on >> kernel address and i assume for any special address any architecture mig= ht have. So >> strictly speaking the access_ok is just a way to fail early and flatout = instead 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. I've dropped the access_ok check and also fixed another bug in the VM = handling. Patches are on their way to the list. Thanks for the comments, Christian. > -Daniel