Re: [PATCH 10/16] drm/gem: implement mmap access management

[Daniel Vetter <daniel@ffwll.ch>]

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@gmail.com>

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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch