On 08/16/2013 07:01 PM, David Herrmann wrote:
Hi
On Fri, Aug 16, 2013 at 5:33 PM, Thomas Hellstrom <thellstrom@xxxxxxxxxx> wrote:
On 08/16/2013 03:19 PM, David Herrmann wrote:
Hi
On Wed, Aug 14, 2013 at 7:35 PM, Thomas Hellstrom <thellstrom@xxxxxxxxxx>
wrote:
(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.
Hi
On Tue, Aug 13, 2013 at 9:38 PM, David Herrmann <dh.herrmann@xxxxxxxxx>
wrote:
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?
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?
Yes, this is how TTM buffer management used to work in older TTM drivers
and how
the codebase for newer device versions will work.
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?
Yes, that sounds fine.
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.
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.
Understood.
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, ...).
Sounds fair enough.
Thanks,
Thomas
Cheers
David
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?
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.
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.
Cc: Thomas Hellstrom <thellstrom@xxxxxxxxxx>
Signed-off-by: David Herrmann <dh.herrmann@xxxxxxxxx>
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
---
drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 29
+++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
index 0e67cf4..4d3f0ae 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
@@ -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)?
*/
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.
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.
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
Otherwise looks OK to me.
Thanks!
David
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
