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önig wrote:
Am 06.08.2014 um 18:08 schrieb Jerome Glisse:
On Wed, Aug 06, 2014 at 08:55:28AM +0200, Christian König wrote:
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.
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 and i am
pretty sure on x86 the VERIFY_WRITE or VERIFY_READ is just flat out ignored.
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 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 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 invalidated by some
mmu_notifier call).
That being said access_ok is kind of useless given that get_user_page will fail on
kernel address and i assume for any special address any architecture might 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
