DRM VMA Access Management

Correctly allow and revoke buffer access on each open/close via the new VMA offset manager. We also need to make vmw_user_dmabuf_reference() correctly increase the vma-allow counter, but it is unused so remove it instead.

Cc Thomas Hellstrom <[email protected]>

Signed-off-by David Herrmann <[email protected]>

---
 drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +17 -13

index 0e67cf4..4d3f0ae 100644
@@ -499,6 +499,12 @@ int vmw_dmabuf_alloc_ioctl(struct drm_device *dev, void *data,
        if (unlikely(ret != 0))
                goto out_no_dmabuf;
 
+       ret = drm_vma_node_allow(&dma_buf->base.vma_node, file_priv->filp);
+       if (ret) {
+               vmw_dmabuf_unreference(&dma_buf);
+               goto out_no_dmabuf;
+       }
+
        rep->handle = handle;
        rep->map_handle = drm_vma_node_offset_addr(&dma_buf->base.vma_node);
        rep->cur_gmr_id = handle;

@@ -517,7 +523,18 @@ int vmw_dmabuf_unref_ioctl(struct drm_device *dev, void *data,
 {
        struct drm_vmw_unref_dmabuf_arg *arg =
            (struct drm_vmw_unref_dmabuf_arg *)data;
+       struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
+       struct vmw_dma_buffer *dma_buf;
+       int ret;
+
+       ret = vmw_user_dmabuf_lookup(tfile, arg->handle, &dma_buf);
+       if (ret)
+               return -EINVAL;
 
+       drm_vma_node_revoke(&dma_buf->base.vma_node, file_priv->filp);
+       vmw_dmabuf_unreference(&dma_buf);
+
+       /* FIXME: is this equivalent to vmw_dmabuf_unreference(dma_buf)? */
        return ttm_ref_object_base_unref(vmw_fpriv(file_priv)->tfile,
                                         arg->handle,
                                         TTM_REF_USAGE);

@@ -551,18 +568,6 @@ int vmw_user_dmabuf_lookup(struct ttm_object_file *tfile,
        return 0;
 }
 
-int vmw_user_dmabuf_reference(struct ttm_object_file *tfile,
-                             struct vmw_dma_buffer *dma_buf)
-{
-       struct vmw_user_dma_buffer *user_bo;
-
-       if (dma_buf->base.destroy != vmw_user_dmabuf_destroy)
-               return -EINVAL;
-
-       user_bo = container_of(dma_buf, struct vmw_user_dma_buffer, dma);
-       return ttm_ref_object_add(tfile, &user_bo->base, TTM_REF_USAGE, NULL);
-}
-
 /*
  * Stream management
  */
--

1.8.3.4

Hi

Quoted 2 lines · David Herrmann

Correctly allow and revoke buffer access on each open/close via the new VMA offset manager.

Quoted 3 lines

We also need to make vmw_user_dmabuf_reference()
correctly increase the vma-allow counter, but it is unused so remove it instead.

Quoted 3 lines

Cc Thomas Hellstrom <[email protected]>

Signed-off-by David Herrmann <[email protected]>

Just as a hint, this patch would allow to remove the "->access_verify()" callback in vmwgfx. No other driver uses it, afaik. I will try to add this in v2.

Regards
David

Quoted 33 lines

---
  drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 29 +++++++++++++++++------------
  1 file changed, 17 insertions(+), 12 deletions(-)

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +13 -0

index 0e67cf4..4d3f0ae 100644
@@ -499,6 +499,12 @@ int vmw_dmabuf_alloc_ioctl(struct drm_device *dev, void *data,
         if (unlikely(ret != 0))
                 goto out_no_dmabuf;
 
+       ret = drm_vma_node_allow(&dma_buf->base.vma_node, file_priv->filp);
+       if (ret) {
+               vmw_dmabuf_unreference(&dma_buf);
+               goto out_no_dmabuf;
+       }
+
         rep->handle = handle;
         rep->map_handle = drm_vma_node_offset_addr(&dma_buf->base.vma_node);
         rep->cur_gmr_id = handle;

@@ -517,7 +523,18 @@ int vmw_dmabuf_unref_ioctl(struct drm_device *dev, void *data,
  {
         struct drm_vmw_unref_dmabuf_arg *arg =
             (struct drm_vmw_unref_dmabuf_arg *)data;
+       struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
+       struct vmw_dma_buffer *dma_buf;
+       int ret;
+
+       ret = vmw_user_dmabuf_lookup(tfile, arg->handle, &dma_buf);
+       if (ret)
+               return -EINVAL;
 

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c int vmw_dmabuf_unref_ioctl(struct drm_device *dev, void *data, · line 537

Quoted 4 lines

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +4 -0

+       drm_vma_node_revoke(&dma_buf->base.vma_node, file_priv->filp);
+       vmw_dmabuf_unreference(&dma_buf);
+
+       /* FIXME: is this equivalent to vmw_dmabuf_unreference(dma_buf)? */

Quoted 17 lines

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +0 -10

         return ttm_ref_object_base_unref(vmw_fpriv(file_priv)->tfile,
                                          arg->handle,
                                          TTM_REF_USAGE);

@@ -551,18 +568,6 @@ int vmw_user_dmabuf_lookup(struct ttm_object_file *tfile,
         return 0;
  }
 
-int vmw_user_dmabuf_reference(struct ttm_object_file *tfile,
-                             struct vmw_dma_buffer *dma_buf)
-{
-       struct vmw_user_dma_buffer *user_bo;
-
-       if (dma_buf->base.destroy != vmw_user_dmabuf_destroy)
-               return -EINVAL;
-
-       user_bo = container_of(dma_buf, struct vmw_user_dma_buffer, dma);
-       return ttm_ref_object_add(tfile, &user_bo->base, TTM_REF_USAGE, NULL);

Quoted 8 lines

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +0 -3

-}
-
  /*
   * Stream management
   */
--

1.8.3.4

(CC'ing the proper people since I'm still on parental leave).

On 08/13/2013 11:44 PM, David Herrmann wrote:

Please see inline comments.

Quoted 5 lines

Hi

Quoted 1 line · David Herrmann

Quoted 2 lines

Quoted 2 lines

Correctly allow and revoke buffer access on each open/close via the new VMA offset manager.

I haven't yet had time to check the access policies of the new VMA offset manager, but anything that is identical or stricter than the current vmwgfx verify_access() would be fine. If it's stricter however, we need to double-check backwards user-space compatibility.

Quoted 4 lines

Quoted 4 lines

We also need to make vmw_user_dmabuf_reference()
correctly increase the vma-allow counter, but it is unused so remove it instead.

IIRC this function or a derivative thereof is used heavily in an upcoming version driver, so if possible, please add a corrected version rather than remove the (currently) unused code. This will trigger a merge error and the upcoming code can be more easily corrected.

Quoted 51 lines

Quoted 4 lines

Cc Thomas Hellstrom <[email protected]>

Signed-off-by David Herrmann <[email protected]>

Just as a hint, this patch would allow to remove the "->access_verify()" callback in vmwgfx. No other driver uses it, afaik. I will try to add this in v2.

Regards
David

Quoted 39 lines

---
  drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 29
+++++++++++++++++------------
  1 file changed, 17 insertions(+), 12 deletions(-)

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +6 -0

index 0e67cf4..4d3f0ae 100644
@@ -499,6 +499,12 @@ int vmw_dmabuf_alloc_ioctl(struct drm_device *dev,
 void *data,
         if (unlikely(ret != 0))
                 goto out_no_dmabuf;
 
+       ret = drm_vma_node_allow(&dma_buf->base.vma_node,
 file_priv->filp);
+       if (ret) {
+               vmw_dmabuf_unreference(&dma_buf);
+               goto out_no_dmabuf;
+       }
+
         rep->handle = handle;

         rep->map_handle =
drm_vma_node_offset_addr(&dma_buf->base.vma_node);
         rep->cur_gmr_id = handle;

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +7 -0

@@ -517,7 +523,18 @@ int vmw_dmabuf_unref_ioctl(struct drm_device *dev,
 void *data,
  {
         struct drm_vmw_unref_dmabuf_arg *arg =
             (struct drm_vmw_unref_dmabuf_arg *)data;
+       struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
+       struct vmw_dma_buffer *dma_buf;
+       int ret;
+
+       ret = vmw_user_dmabuf_lookup(tfile, arg->handle, &dma_buf);
+       if (ret)
+               return -EINVAL;
 

Quoted 5 lines

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c int vmw_dmabuf_unref_ioctl(struct drm_device *dev, void *data,

Quoted 5 lines

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +4 -0

+       drm_vma_node_revoke(&dma_buf->base.vma_node, file_priv->filp);
+       vmw_dmabuf_unreference(&dma_buf);
+
+       /* FIXME: is this equivalent to vmw_dmabuf_unreference(dma_buf)?
 */

No. A ttm ref object is rather similar to a generic GEM object, only that it's generic in the sense that it is not restricted to buffers, and can make any desired object visible to user-space. So translated the below code removes a reference that the arg->handle holds on the "gem" object, potentially destroying the whole object in which the "gem" object is embedded.

Quoted 22 lines

Quoted 22 lines

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +0 -0

         return ttm_ref_object_base_unref(vmw_fpriv(file_priv)->tfile,

                                          arg->handle,
                                          TTM_REF_USAGE);

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +0 -12

@@ -551,18 +568,6 @@ int vmw_user_dmabuf_lookup(struct ttm_object_file
 *tfile,
         return 0;
  }
 
-int vmw_user_dmabuf_reference(struct ttm_object_file *tfile,
-                             struct vmw_dma_buffer *dma_buf)
-{
-       struct vmw_user_dma_buffer *user_bo;
-
-       if (dma_buf->base.destroy != vmw_user_dmabuf_destroy)
-               return -EINVAL;
-
-       user_bo = container_of(dma_buf, struct vmw_user_dma_buffer, dma);
-       return ttm_ref_object_add(tfile, &user_bo->base, TTM_REF_USAGE,
 NULL);
-}
-

Quoted 6 lines

Quoted 6 lines

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +0 -0

  /*

* Stream management
*/
--
1.8.3.4

Otherwise looks OK to me.

Hi

Quoted 11 lines · Thomas Hellstrom

(CC'ing the proper people since I'm still on parental leave).

On 08/13/2013 11:44 PM, David Herrmann wrote:

Please see inline comments.

Quoted 4 lines

Hi

On Tue, Aug 13, 2013 at 9:38 PM, David Herrmann <[email protected]> wrote:

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

Correctly allow and revoke buffer access on each open/close via the new VMA offset manager.

I haven't yet had time to check the access policies of the new VMA offset manager, but anything that is identical or stricter than the current vmwgfx verify_access() would be fine. If it's stricter however, we need to double-check backwards user-space compatibility.

My patches make vmw_dmabuf_alloc_ioctl() add the caller's open-file (file*) to the list of allowed users of the new bo. vmw_dmabuf_unref_ioctl() removes it again. I haven't seen any way to pass a user-dmabuf to another user so there is currently at most one user for a vmw_dmabuf. vmw_user_dmabuf_reference() looks like it is intended exactly for this case so it would have to add the file* of the caller to the list of allowed users. I will change that in v2. This means every user who gets a handle for the buffer (like gem_open) will be added to the allowed users. For TTM-object currently only a single user is allowed.

So I replace vmw_user_bo->base.tfile with a list (actually rbtree) of allowed files. So more than one user can have access. This, however, breaks the "shareable" attribute which I wasn't aware of. As far as I can see, "shareable" is only used by vmwgfx_surface.c and can be set by userspace to allow arbitrary processes to map this buffer (sounds like a security issue similar to gem flink).
I actually think we can replace the "shareable" attribute with proper access-management in the vma-manager. But first I'd need to know whether "shareable = true" is actually used by vmwgfx user-space and how buffers are shared? Do you simply pass the mmap offset between processes? Or do you pass some handle?

If you really pass mmap offsets in user-space and rely on this, I guess there is no way I can make vmwgfx use the vma-manager access management. I will have to find a way to work around it or move the "shareable" flag to ttm_bo.

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

We also need to make vmw_user_dmabuf_reference()
correctly increase the vma-allow counter, but it is unused so remove it instead.

IIRC this function or a derivative thereof is used heavily in an upcoming version driver, so if possible, please add a corrected version rather than remove the (currently) unused code. This will trigger a merge error and the upcoming code can be more easily corrected.

I will do so.

Quoted 48 lines

Quoted 48 lines

Quoted 2 lines

Cc Thomas Hellstrom <[email protected]>

Signed-off-by David Herrmann <[email protected]>

Just as a hint, this patch would allow to remove the "->access_verify()" callback in vmwgfx. No other driver uses it, afaik. I will try to add this in v2.

Regards
David

Quoted 39 lines

---
   drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 29
+++++++++++++++++------------
   1 file changed, 17 insertions(+), 12 deletions(-)

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +6 -0

index 0e67cf4..4d3f0ae 100644
@@ -499,6 +499,12 @@ int vmw_dmabuf_alloc_ioctl(struct drm_device *dev,
 void *data,
          if (unlikely(ret != 0))
                  goto out_no_dmabuf;
 
+       ret = drm_vma_node_allow(&dma_buf->base.vma_node,
 file_priv->filp);
+       if (ret) {
+               vmw_dmabuf_unreference(&dma_buf);
+               goto out_no_dmabuf;
+       }
+
          rep->handle = handle;

          rep->map_handle =
drm_vma_node_offset_addr(&dma_buf->base.vma_node);
          rep->cur_gmr_id = handle;

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +7 -0

@@ -517,7 +523,18 @@ int vmw_dmabuf_unref_ioctl(struct drm_device *dev,
 void *data,
   {
          struct drm_vmw_unref_dmabuf_arg *arg =
              (struct drm_vmw_unref_dmabuf_arg *)data;
+       struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
+       struct vmw_dma_buffer *dma_buf;
+       int ret;
+
+       ret = vmw_user_dmabuf_lookup(tfile, arg->handle, &dma_buf);
+       if (ret)
+               return -EINVAL;
 

Quoted 12 lines

Quoted 5 lines

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c int vmw_dmabuf_unref_ioctl(struct drm_device *dev, void *data,

Quoted 5 lines

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +4 -0

+       drm_vma_node_revoke(&dma_buf->base.vma_node, file_priv->filp);
+       vmw_dmabuf_unreference(&dma_buf);
+
+       /* FIXME: is this equivalent to vmw_dmabuf_unreference(dma_buf)?
 */

No. A ttm ref object is rather similar to a generic GEM object, only that it's generic in the sense that it is not restricted to buffers, and can make any desired object visible to user-space. So translated the below code removes a reference that the arg->handle holds on the "gem" object, potentially destroying the whole object in which the "gem" object is embedded.

So I actually need both lookups, vmw_user_dmabuf_lookup() and the lookup in ttm_ref_object_base_unref()? Ugh.. but ok, I will leave the function then as it is now but remove the comment.

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +0 -0

          return ttm_ref_object_base_unref(vmw_fpriv(file_priv)->tfile,

                                           arg->handle,
                                           TTM_REF_USAGE);

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +0 -11

@@ -551,18 +568,6 @@ int vmw_user_dmabuf_lookup(struct ttm_object_file
 *tfile,
          return 0;
   }
 
-int vmw_user_dmabuf_reference(struct ttm_object_file *tfile,
-                             struct vmw_dma_buffer *dma_buf)
-{
-       struct vmw_user_dma_buffer *user_bo;
-
-       if (dma_buf->base.destroy != vmw_user_dmabuf_destroy)
-               return -EINVAL;
-
-       user_bo = container_of(dma_buf, struct vmw_user_dma_buffer, dma);
-       return ttm_ref_object_add(tfile, &user_bo->base, TTM_REF_USAGE,
 NULL);
-}

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +0 -1

-
   /*

* Stream management
*/
--
1.8.3.4

Otherwise looks OK to me.

Thanks!
David

Quoted 39 lines · David Herrmann

Hi

Quoted 17 lines · Thomas Hellstrom

(CC'ing the proper people since I'm still on parental leave).

On 08/13/2013 11:44 PM, David Herrmann wrote:

Please see inline comments.

Quoted 7 lines

Hi

Quoted 3 lines · David Herrmann

Correctly allow and revoke buffer access on each open/close via the new VMA offset manager.

Quoted 5 lines

I haven't yet had time to check the access policies of the new VMA offset manager, but anything that is identical or stricter than the current vmwgfx
verify_access() would be fine. If it's stricter however, we need to double-check backwards user-space compatibility.

My patches make vmw_dmabuf_alloc_ioctl() add the caller's open-file (file*) to the list of allowed users of the new bo. vmw_dmabuf_unref_ioctl() removes it again. I haven't seen any way to pass a user-dmabuf to another user so there is currently at most one user for a vmw_dmabuf. vmw_user_dmabuf_reference() looks like it is intended exactly for this case so it would have to add the file* of the caller to the list of allowed users. I will change that in v2. This means every user who gets a handle for the buffer (like gem_open) will be added to the allowed users. For TTM-object currently only a single user is allowed.

Quoted 11 lines

So I replace vmw_user_bo->base.tfile with a list (actually rbtree) of allowed files. So more than one user can have access. This, however, breaks the "shareable" attribute which I wasn't aware of. As far as I can see, "shareable" is only used by vmwgfx_surface.c and can be set by userspace to allow arbitrary processes to map this buffer (sounds like a security issue similar to gem flink).
I actually think we can replace the "shareable" attribute with proper access-management in the vma-manager. But first I'd need to know whether "shareable = true" is actually used by vmwgfx user-space and how buffers are shared? Do you simply pass the mmap offset between processes? Or do you pass some handle?

Buffer- and surface sharing is done by passing an opaque (not mmap) handle. A process intending to map the shared buffer must obtain the map offset through a
vmw_user_dmabuf_reference() call, and that only works if the buffer is "shareable".

mmap offsets are never passed between processes, but valid only if obtained directly
from the kernel driver.

This means that currently buffer mapping should have the same access restriction as the
X server imposes on DRI clients; If a process is allowed to open the drm device, it also has
map access to all "shareable" objects, which is a security hole in the sense that verify_access() should
really check that the caller, if not the buffer owner, is also authenticated.

The reason verify_access() is there is to make the TTM buffer object transparent to how it is exported
to user space (GEM or TTM objects). Apparently the GEM TTM drivers have ignored this hook for some unknown
reason.

Some ideas:
1) Rather than having a list of allowable files on each buffer object, perhaps we should have a user and a group and
a set of permissions (for user, group and system) more like how files are handled?

2) Rather than imposing a security policy in the vma manager, could we perhaps have a set a utility functions that
are called through verify_access(). Each driver could then have a wrapper to gather the needed information and
hand it over to the VMA manager?

Quoted 84 lines

If you really pass mmap offsets in user-space and rely on this, I guess there is no way I can make vmwgfx use the vma-manager access management. I will have to find a way to work around it or move the "shareable" flag to ttm_bo.

Quoted 10 lines

Quoted 3 lines

Quoted 3 lines

We also need to make vmw_user_dmabuf_reference()
correctly increase the vma-allow counter, but it is unused so remove it instead.

IIRC this function or a derivative thereof is used heavily in an upcoming version driver, so if possible, please add a corrected version rather than
remove the (currently) unused code. This will trigger a merge error and the
upcoming code can be more easily corrected.

I will do so.

Quoted 49 lines

Quoted 49 lines

Quoted 2 lines

Cc Thomas Hellstrom <[email protected]>

Signed-off-by David Herrmann <[email protected]>

Just as a hint, this patch would allow to remove the "->access_verify()" callback in vmwgfx. No other driver uses it, afaik. I will try to add this in v2.

Regards
David

Quoted 39 lines

---
   drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 29
+++++++++++++++++------------
   1 file changed, 17 insertions(+), 12 deletions(-)

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +6 -0

index 0e67cf4..4d3f0ae 100644
@@ -499,6 +499,12 @@ int vmw_dmabuf_alloc_ioctl(struct drm_device *dev,
 void *data,
          if (unlikely(ret != 0))
                  goto out_no_dmabuf;
 
+       ret = drm_vma_node_allow(&dma_buf->base.vma_node,
 file_priv->filp);
+       if (ret) {
+               vmw_dmabuf_unreference(&dma_buf);
+               goto out_no_dmabuf;
+       }
+
          rep->handle = handle;

          rep->map_handle =
drm_vma_node_offset_addr(&dma_buf->base.vma_node);
          rep->cur_gmr_id = handle;

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +7 -0

@@ -517,7 +523,18 @@ int vmw_dmabuf_unref_ioctl(struct drm_device *dev,
 void *data,
   {
          struct drm_vmw_unref_dmabuf_arg *arg =
              (struct drm_vmw_unref_dmabuf_arg *)data;
+       struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
+       struct vmw_dma_buffer *dma_buf;
+       int ret;
+
+       ret = vmw_user_dmabuf_lookup(tfile, arg->handle, &dma_buf);
+       if (ret)
+               return -EINVAL;
 

Quoted 15 lines

Quoted 6 lines

Quoted 6 lines

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +4 -0

+       drm_vma_node_revoke(&dma_buf->base.vma_node, file_priv->filp);
+       vmw_dmabuf_unreference(&dma_buf);
+
+       /* FIXME: is this equivalent to

vmw_dmabuf_unreference(dma_buf)?
*/

No. A ttm ref object is rather similar to a generic GEM object, only that it's generic in the sense that it is not restricted to buffers, and can make
any desired object visible to user-space. So translated the below code removes a reference that the arg->handle holds on the "gem" object, potentially destroying the whole object in which the "gem" object is embedded.

Quoted 3 lines

So I actually need both lookups, vmw_user_dmabuf_lookup() and the lookup in ttm_ref_object_base_unref()? Ugh.. but ok, I will leave the function then as it is now but remove the comment.

Yes. This seems odd, but IIRC the lookups are from different hash tables. The unref() call
makes a lookup in a hash table private to the file.

Quoted 33 lines

Quoted 22 lines

Quoted 22 lines

Quoted 22 lines

          return ttm_ref_object_base_unref(vmw_fpriv(file_priv)->tfile,
                                           arg->handle,
                                           TTM_REF_USAGE);

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +0 -11

@@ -551,18 +568,6 @@ int vmw_user_dmabuf_lookup(struct ttm_object_file
 *tfile,
          return 0;
   }
 
-int vmw_user_dmabuf_reference(struct ttm_object_file *tfile,
-                             struct vmw_dma_buffer *dma_buf)
-{
-       struct vmw_user_dma_buffer *user_bo;
-
-       if (dma_buf->base.destroy != vmw_user_dmabuf_destroy)
-               return -EINVAL;
-
-       user_bo = container_of(dma_buf, struct vmw_user_dma_buffer,
 dma);
-       return ttm_ref_object_add(tfile, &user_bo->base, TTM_REF_USAGE,
 NULL);
-}

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +0 -1

-

   /*
    * Stream management
    */

--
1.8.3.4

Otherwise looks OK to me.

Thanks!
David

Hi

Quoted 47 lines · Thomas Hellstrom

Quoted 45 lines · David Herrmann

Hi

Quoted 11 lines · Thomas Hellstrom

(CC'ing the proper people since I'm still on parental leave).

On 08/13/2013 11:44 PM, David Herrmann wrote:

Please see inline comments.

Quoted 4 lines

Hi

On Tue, Aug 13, 2013 at 9:38 PM, David Herrmann <[email protected]> wrote:

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Correctly allow and revoke buffer access on each open/close via the new VMA offset manager.

I haven't yet had time to check the access policies of the new VMA offset manager, but anything that is identical or stricter than the current vmwgfx
verify_access() would be fine. If it's stricter however, we need to double-check backwards user-space compatibility.

My patches make vmw_dmabuf_alloc_ioctl() add the caller's open-file (file*) to the list of allowed users of the new bo. vmw_dmabuf_unref_ioctl() removes it again. I haven't seen any way to pass a user-dmabuf to another user so there is currently at most one user for a vmw_dmabuf. vmw_user_dmabuf_reference() looks like it is intended exactly for this case so it would have to add the file* of the caller to the list of allowed users. I will change that in v2. This means every user who gets a handle for the buffer (like gem_open) will be added to the allowed users. For TTM-object currently only a single user is allowed.

So I replace vmw_user_bo->base.tfile with a list (actually rbtree) of allowed files. So more than one user can have access. This, however, breaks the "shareable" attribute which I wasn't aware of. As far as I can see, "shareable" is only used by vmwgfx_surface.c and can be set by userspace to allow arbitrary processes to map this buffer (sounds like a security issue similar to gem flink).
I actually think we can replace the "shareable" attribute with proper access-management in the vma-manager. But first I'd need to know whether "shareable = true" is actually used by vmwgfx user-space and how buffers are shared? Do you simply pass the mmap offset between processes? Or do you pass some handle?

Quoted 5 lines

Buffer- and surface sharing is done by passing an opaque (not mmap) handle. A process intending to map the shared buffer must obtain the map offset through a
vmw_user_dmabuf_reference() call, and that only works if the buffer is "shareable".

Ugh? That's not true. At least in upstream vmwgfx vmw_user_dmabuf_reference() is unused. Maybe you have access to some newer codebase?

Anyway, I can easily make this function call drm_vma_node_allow() and then newer vmwgfx additions will work just fine. This means, every user who calls vmw_user_dmabuf_reference() will then also be allowed to mmap that buffer. But users who do not own a handle (that is, they didn't call vmw_user_dmabuf_reference() or they dropped the reference via vmw_user_dmabuf_unref_ioctl()) will get -EACCES if they try to mmap the buffer.

This is an extension to how it currently works, so I doubt that it breaks any user-space. Is that fine for vmwgfx?

Quoted 4 lines

mmap offsets are never passed between processes, but valid only if obtained directly
from the kernel driver.

Good to hear. That means this patch doesn't break any existing userspace.

Quoted 8 lines

This means that currently buffer mapping should have the same access restriction as the
X server imposes on DRI clients; If a process is allowed to open the drm device, it also has
map access to all "shareable" objects, which is a security hole in the sense that verify_access() should
really check that the caller, if not the buffer owner, is also authenticated.

I actually don't care for DRI. This series tries to fix exactly that. I don't want that. Users with DRM access shouldn't be able to map arbitrary buffers. Instead, users should only be able to map buffers that they own a handle for. Access management for handles is an orthogonal problem that this series does not change. dma-buf does a pretty good job there, anyway.

Quoted 6 lines

The reason verify_access() is there is to make the TTM buffer object transparent to how it is exported
to user space (GEM or TTM objects). Apparently the GEM TTM drivers have ignored this hook for some unknown
reason.

I don't think that we need any extended access-management here. Why would we ever need different access-modes for mmap than for handles? This series reduces mmap() access-management to handle-access-management. That is, the right to mmap() a buffer is now bound to a buffer handle. If you don't own a handle, you cannot mmap the buffer. But if you own a handle, you're always allowed to mmap the buffer. I think this should be the policy to go for, or am I missing something?

That's also why I think verify_access() is not needed at all. Drivers shouldn't care for mmap() access, instead they should take care only privileged users get handles (whatever they do to guarantee that, gem-flink, dma-buf, ...).

Cheers
David

Quoted 131 lines

Some ideas:
1) Rather than having a list of allowable files on each buffer object, perhaps we should have a user and a group and
a set of permissions (for user, group and system) more like how files are handled?

2) Rather than imposing a security policy in the vma manager, could we perhaps have a set a utility functions that
are called through verify_access(). Each driver could then have a wrapper to gather the needed information and
hand it over to the VMA manager?

Quoted 78 lines

If you really pass mmap offsets in user-space and rely on this, I guess there is no way I can make vmwgfx use the vma-manager access management. I will have to find a way to work around it or move the "shareable" flag to ttm_bo.

Quoted 9 lines

Quoted 3 lines

Quoted 3 lines

We also need to make vmw_user_dmabuf_reference()
correctly increase the vma-allow counter, but it is unused so remove it instead.

IIRC this function or a derivative thereof is used heavily in an upcoming version driver, so if possible, please add a corrected version rather than
remove the (currently) unused code. This will trigger a merge error and the
upcoming code can be more easily corrected.

I will do so.

Quoted 48 lines

Quoted 48 lines

Quoted 2 lines

Cc Thomas Hellstrom <[email protected]>

Signed-off-by David Herrmann <[email protected]>

Just as a hint, this patch would allow to remove the "->access_verify()" callback in vmwgfx. No other driver uses it, afaik. I will try to add this in v2.

Regards
David

Quoted 39 lines

---
    drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 29
+++++++++++++++++------------
    1 file changed, 17 insertions(+), 12 deletions(-)

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +6 -0

index 0e67cf4..4d3f0ae 100644
@@ -499,6 +499,12 @@ int vmw_dmabuf_alloc_ioctl(struct drm_device *dev,
 void *data,
           if (unlikely(ret != 0))
                   goto out_no_dmabuf;
 
+       ret = drm_vma_node_allow(&dma_buf->base.vma_node,
 file_priv->filp);
+       if (ret) {
+               vmw_dmabuf_unreference(&dma_buf);
+               goto out_no_dmabuf;
+       }
+
           rep->handle = handle;

           rep->map_handle =
drm_vma_node_offset_addr(&dma_buf->base.vma_node);
           rep->cur_gmr_id = handle;

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +7 -0

@@ -517,7 +523,18 @@ int vmw_dmabuf_unref_ioctl(struct drm_device *dev,
 void *data,
    {
           struct drm_vmw_unref_dmabuf_arg *arg =
               (struct drm_vmw_unref_dmabuf_arg *)data;
+       struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
+       struct vmw_dma_buffer *dma_buf;
+       int ret;
+
+       ret = vmw_user_dmabuf_lookup(tfile, arg->handle, &dma_buf);
+       if (ret)
+               return -EINVAL;
 

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +4 -0

+       drm_vma_node_revoke(&dma_buf->base.vma_node, file_priv->filp);
+       vmw_dmabuf_unreference(&dma_buf);
+
+       /* FIXME: is this equivalent to

vmw_dmabuf_unreference(dma_buf)?
*/

No. A ttm ref object is rather similar to a generic GEM object, only that it's generic in the sense that it is not restricted to buffers, and can make
any desired object visible to user-space. So translated the below code removes a reference that the arg->handle holds on the "gem" object, potentially destroying the whole object in which the "gem" object is embedded.

Quoted 3 lines

So I actually need both lookups, vmw_user_dmabuf_lookup() and the lookup in ttm_ref_object_base_unref()? Ugh.. but ok, I will leave the function then as it is now but remove the comment.

Yes. This seems odd, but IIRC the lookups are from different hash tables. The unref() call
makes a lookup in a hash table private to the file.

Quoted 31 lines

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

           return ttm_ref_object_base_unref(vmw_fpriv(file_priv)->tfile,
                                            arg->handle,
                                            TTM_REF_USAGE);

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +0 -11

@@ -551,18 +568,6 @@ int vmw_user_dmabuf_lookup(struct ttm_object_file
 *tfile,
           return 0;
    }
 
-int vmw_user_dmabuf_reference(struct ttm_object_file *tfile,
-                             struct vmw_dma_buffer *dma_buf)
-{
-       struct vmw_user_dma_buffer *user_bo;
-
-       if (dma_buf->base.destroy != vmw_user_dmabuf_destroy)
-               return -EINVAL;
-
-       user_bo = container_of(dma_buf, struct vmw_user_dma_buffer,
 dma);
-       return ttm_ref_object_add(tfile, &user_bo->base, TTM_REF_USAGE,
 NULL);
-}

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +0 -1

-

    /*
     * Stream management
     */

--
1.8.3.4

Otherwise looks OK to me.

Thanks!
David

Implement automatic access management for mmap offsets for all GEM drivers. This prevents user-space applications from "guessing" GEM BO offsets and accessing buffers which they don't own.

TTM drivers or other modesetting drivers with custom mm handling might make use of GEM but don't need its mmap helpers. To avoid unnecessary overhead, we limit GEM access management to drivers using DRIVER_GEM_MMAP. So for TTM drivers GEM will not call any *_allow() or *_revoke() helpers.

Signed-off-by David Herrmann <[email protected]>

---
 Documentation/DocBook/drm.tmpl | 13 +++++++++++++
 drivers/gpu/drm/drm_gem.c      | 36 ++++++++++++++++++++++++++++++++----
 include/drm/drmP.h             |  1 +
 3 files changed, 46 insertions(+), 4 deletions(-)

Documentation/DocBook/drm.tmpl +13 -0

index 87e22ec..a388749 100644
@@ -223,6 +223,19 @@
             </para></listitem>
           </varlistentry>
           <varlistentry>
+            <term>DRIVER_GEM_MMAP</term>
+            <listitem><para>
+              Driver uses default GEM mmap helpers. This flag guarantees that
+              GEM core takes care of buffer access management and prevents
+              unprivileged users from mapping random buffers. This flag should
+              only be set by GEM-only drivers that use the drm_gem_mmap_*()
+              helpers directly. TTM, on the other hand, takes care of access
+              management itself, even though drivers might use DRIVER_GEM and
+              TTM at the same time. See the DRM VMA Offset Manager interface for
+              more information on buffer mmap() access management.
+            </para></listitem>
+          </varlistentry>
+          <varlistentry>
             <term>DRIVER_MODESET</term>
             <listitem><para>
               Driver supports mode setting interfaces (KMS).

drivers/gpu/drm/drm_gem.c +7 -0

index 7043d89..887274f 100644
@@ -236,6 +236,9 @@ drm_gem_handle_delete(struct drm_file *filp, u32 handle)
 
 	drm_gem_remove_prime_handles(obj, filp);
 
+	if (drm_core_check_feature(dev, DRIVER_GEM_MMAP))
+		drm_vma_node_revoke(&obj->vma_node, filp->filp);
+
 	if (dev->driver->gem_close_object)
 		dev->driver->gem_close_object(obj, filp);
 	drm_gem_object_handle_unreference_unlocked(obj);

@@ -288,15 +291,26 @@ drm_gem_handle_create(struct drm_file *file_priv,
 
 	drm_gem_object_handle_reference(obj);
 
+	if (drm_core_check_feature(dev, DRIVER_GEM_MMAP)) {
+		ret = drm_vma_node_allow(&obj->vma_node, file_priv->filp);
+		if (ret)
+			goto err_handle;

drivers/gpu/drm/drm_gem.c +21 -0

 	}
 
 	return 0;
+
+err_node:
+	if (drm_core_check_feature(dev, DRIVER_GEM_MMAP))
+		drm_vma_node_revoke(&obj->vma_node, file_priv->filp);
+err_handle:
+	drm_gem_handle_delete(file_priv, *handlep);
+	return ret;
 }
 EXPORT_SYMBOL(drm_gem_handle_create);
 

@@ -486,6 +500,9 @@ drm_gem_object_release_handle(int id, void *ptr, void *data)
 
 	drm_gem_remove_prime_handles(obj, file_priv);
 
+	if (drm_core_check_feature(dev, DRIVER_GEM_MMAP))
+		drm_vma_node_revoke(&obj->vma_node, file_priv->filp);
+
 	if (dev->driver->gem_close_object)
 		dev->driver->gem_close_object(obj, file_priv);
 

@@ -610,6 +627,10 @@ EXPORT_SYMBOL(drm_gem_vm_close);
  * the GEM object is not looked up based on its fake offset. To implement the
  * DRM mmap operation, drivers should use the drm_gem_mmap() function.
  *
+ * drm_gem_mmap_obj() assumes the user is granted access to the buffer while
+ * drm_gem_mmap() prevents unprivileged users from mapping random objects. So
+ * callers must verify access restrictions before calling this helper.
+ *
  * NOTE: This function has to be protected with dev->struct_mutex
  *
  * Return 0 or success or -EINVAL if the object size is smaller than the VMA

@@ -658,6 +679,9 @@ EXPORT_SYMBOL(drm_gem_mmap_obj);
  * Look up the GEM object based on the offset passed in (vma->vm_pgoff will
  * contain the fake offset we created when the GTT map ioctl was called on
  * the object) and map it with a call to drm_gem_mmap_obj().
+ *
+ * If the caller is not granted access to the buffer object, the mmap will fail
+ * with EACCES. Please see DRIVER_GEM_MMAP for more information.
  */
 int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
 {

@@ -678,6 +702,10 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
 	if (!node) {
 		mutex_unlock(&dev->struct_mutex);
 		return drm_mmap(filp, vma);
+	} else if (drm_core_check_feature(dev, DRIVER_GEM_MMAP) &&
+		   !drm_vma_node_is_allowed(node, filp)) {
+		mutex_unlock(&dev->struct_mutex);
+		return -EACCES;
 	}
 
 	obj = container_of(node, struct drm_gem_object, vma_node);

include/drm/drmP.h +1 -0

index 3ecdde6..d51accd 100644
@@ -152,6 +152,7 @@ int drm_err(const char *func, const char *format, ...);
 #define DRIVER_GEM         0x1000
 #define DRIVER_MODESET     0x2000
 #define DRIVER_PRIME       0x4000
+#define DRIVER_GEM_MMAP    0x8000
 
 #define DRIVER_BUS_PCI 0x1
 #define DRIVER_BUS_PLATFORM 0x2

Signature · 6 lines

1.8.3.4

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 10 lines · David Herrmann

Implement automatic access management for mmap offsets for all GEM drivers. This prevents user-space applications from "guessing" GEM BO offsets and accessing buffers which they don't own.

TTM drivers or other modesetting drivers with custom mm handling might make use of GEM but don't need its mmap helpers. To avoid unnecessary overhead, we limit GEM access management to drivers using DRIVER_GEM_MMAP. So for TTM drivers GEM will not call any *_allow() or *_revoke() helpers.

Signed-off-by David Herrmann <[email protected]>

Overall I'm not sold why we need the GEM_MMAP feature flag. Drivers that don't use drm_gem_mmap shouldn't get hurt with it if we ignore the little bit of useless overhead due to obj->vma_node. But they already have that anyway with obj->vma, so meh. And more incentives to move ttm over to the gem object manager completely ;-)

One comment below.

Set DRIVER_GEM_MMAP to make GEM core track buffer accesses via the DRM VMA Offset Manager.

Cc Rob Clark <[email protected]>

Signed-off-by David Herrmann <[email protected]>

---
 drivers/gpu/drm/omapdrm/omap_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/gpu/drm/omapdrm/omap_drv.c +2 -2

index 2f9e22e..700e71f 100644
@@ -605,7 +605,8 @@ static const struct file_operations omapdriver_fops = {
 
 static struct drm_driver omap_drm_driver = {
                .driver_features =
-                               DRIVER_HAVE_IRQ | DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME,
+                               DRIVER_HAVE_IRQ | DRIVER_MODESET | DRIVER_GEM |
+                               DRIVER_PRIME | DRIVER_GEM_MMAP,
                .load = dev_load,
                .unload = dev_unload,
                .open = dev_open,
--

1.8.3.4