Re: [PATCH 08/16] drm/vmwgfx: implement mmap access managament

[Thomas Hellstrom <thellstrom@vmware.com>]

On 08/16/2013 07:01 PM, David Herrmann wrote:
> Hi
>
> On Fri, Aug 16, 2013 at 5:33 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
>> On 08/16/2013 03:19 PM, David Herrmann wrote:
>>> Hi
>>>
>>> On Wed, Aug 14, 2013 at 7:35 PM, Thomas Hellstrom <thellstrom@vmware.com>
>>> 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@gmail.com>
>>>>> 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@vmware.com>
>>>>>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>>>>> 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