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.

		/* ->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->vm_file = get_file(file);
		error = call_mmap(file, vma);

Cheers, Daniel

Christian.

Jason