On Tue, Aug 13, 2013 at 09:38:31PM +0200, David Herrmann wrote:
> 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 <dh.herrmann@xxxxxxxxx>
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.
Cheers, Daniel
> ---
> Documentation/DocBook/drm.tmpl | 13 +++++++++++++
> drivers/gpu/drm/drm_gem.c | 36 ++++++++++++++++++++++++++++++++----
> include/drm/drmP.h | 1 +
> 3 files changed, 46 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 87e22ec..a388749 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -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).
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 7043d89..887274f 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -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;
> + }
> +
> if (dev->driver->gem_open_object) {
> ret = dev->driver->gem_open_object(obj, file_priv);
> - if (ret) {
> - drm_gem_handle_delete(file_priv, *handlep);
> - return ret;
> - }
> + if (ret)
> + goto err_node;
The error handling cleanup changes here aren't required since
handle_delete will dtrt anyway. Or at least I hope it does ;-)
-Daniel
> }
>
> 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);
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 3ecdde6..d51accd 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -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
> --
> 1.8.3.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
