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? 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?
> 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.
> 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
> 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
