Changing vma->vm_file in dma_buf_mmap()

[Christian König]

Am 16.09.20 um 11:53 schrieb Daniel Vetter:
> On Mon, Sep 14, 2020 at 08:26:47PM +0200, Christian König wrote:
>> Am 14.09.20 um 16:06 schrieb Jason Gunthorpe:
>>> On Mon, Sep 14, 2020 at 03:30:47PM +0200, Christian König wrote:
>>>> Am 14.09.20 um 15:29 schrieb Christian König:
>>>>> Hi Andrew,
>>>>>
>>>>> I'm the new DMA-buf maintainer and Daniel and others came up with
>>>>> patches extending the use of the dma_buf_mmap() function.
>>>>>
>>>>> Now this function is doing something a bit odd by changing the
>>>>> vma->vm_file while installing a VMA in the mmap() system call
>>> It doesn't look obviously safe as mmap_region() has an interesting mix
>>> of file and vma->file
>>>
>>> Eg it calls mapping_unmap_writable() using both routes
>> Thanks for the hint, going to take a look at that code tomorrow.
>>
>>> What about security? Is it OK that some other random file, maybe in
>>> another process, is being linked to this mmap?
>> Good question, I have no idea. That's why I send out this mail.
>>
>>>>> The background here is that DMA-buf allows device drivers to
>>>>> export buffer which are then imported into another device
>>>>> driver. The mmap() handler of the importing device driver then
>>>>> find that the pgoff belongs to the exporting device and so
>>>>> redirects the mmap() call there.
>>> So the pgoff is some virtualized thing?
>> Yes, absolutely.
> Maybe notch more context. Conceptually the buffer objects we use to manage
> gpu memory are all stand-alone objects, internally refcounted and
> everything. And if you export them as a dma-buf, then they are indeed
> stand-alone file descriptors like any other.
>
> But within the driver, we generally need thousands of these, and that
> tends to bring fd exhaustion problems with it. That's why all the private
> buffer objects which aren't shared with other process or other drivers are
> handles only valid for a specific fd instance of the drm chardev (each
> open gets their own namespace), and only for ioctls done on that chardev.
> And for mmap we assign fake (but unique across all open fd on it) offsets
> within the overall chardev. Hence all the pgoff mangling and re-mangling.
>
> Now for unmap_mapping_range we'd like it to find all such fake offset
> aliases pointing at the one underlying buffer object:
> - mmap on the dma-buf fd, at offset 0
> - mmap on the drm chardev where the buffer was originally allocated, at some unique offset
> - mmap on the drm chardev where the buffer was imported, again at some
>    (likely) different unique (for that chardev) offset.
>
> So to make unmap_mapping_range work across the entire delegation change
> we'd actually need to change the vma->vma_file and pgoff twice:
> - once when forwarding from the importing drm chardev to the dma-buf
> - once when forwarding from the dma-buf to the exported drm chardev fake
>    offset, which (mostly for historical reasons) is considered the
>    canonical fake offset
>
> We can't really do the delegation in userspace because:
> - the importer might not have access to the exporters drm chardev, it only
>    gets the dma-buf. If we'd give it the underlying drm chardev it could do
>    stuff like issue rendering commands, breaking the access model.
> - the dma-buf fd is only used to establish the sharing, once it's imported
>    everywhere it generally gets closed. Userspace could re-export it and
>    then call mmap on that, but feels a bit contrived.
> - especially on SoC platforms this has already become uapi. It's not a big
>    problem because the drivers that really need unmap_mapping_range to work
>    are the big gpu drivers with discrete vram, where mappings need to be
>    invalidate when moving buffer objects in/out of vram.
>
> Hence why we'd like to be able to forward aliasing mappings and adjust the
> file and pgoff, while hopefully everything keeps working. I thought this
> would work, but Christian noticed it doesn't really.

Well to be clear I'm still not sure if that works or not :)

But Jason pointed me to the right piece of code. See this comment in in 
mmap_region():

> 		/* ->mmap() can change vma->vm_file, but must guarantee that
> * vma_link() below can deny write-access if VM_DENYWRITE is set
> * and map writably if VM_SHARED is set. This usually means the
> * new file must not have been exposed to user-space, yet.
> */
> 		vma <https://elixir.bootlin.com/linux/v5.9-rc5/C/ident/vma>->vm_file 
> <https://elixir.bootlin.com/linux/v5.9-rc5/C/ident/vm_file>  =  get_file 
> <https://elixir.bootlin.com/linux/v5.9-rc5/C/ident/get_file>(file 
> <https://elixir.bootlin.com/linux/v5.9-rc5/C/ident/file>);
> 		error  =  call_mmap 
> <https://elixir.bootlin.com/linux/v5.9-rc5/C/ident/call_mmap>(file 
> <https://elixir.bootlin.com/linux/v5.9-rc5/C/ident/file>,  vma <https://elixir.bootlin.com/linux/v5.9-rc5/C/ident/vma>);

So changing vma->vm_file is allowed at least under certain circumstances.

Only the "file must not have been exposed to user-space, yet" part still 
needs double checking. Currently working on that.

Regards,
Christian.

>
> Cheers, Daniel
>
>
>> Christian.
>>
>>> Jason

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200916/96686f9b/attachment.htm>