| Message ID | 1376422717-12229-11-git-send-email-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
Hi On Tue, Aug 13, 2013 at 11:05 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > 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. I fixed both, the error-path and GEM_MMAP. Thanks! Cheers David
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; } 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
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> --- Documentation/DocBook/drm.tmpl | 13 +++++++++++++ drivers/gpu/drm/drm_gem.c | 36 ++++++++++++++++++++++++++++++++---- include/drm/drmP.h | 1 + 3 files changed, 46 insertions(+), 4 deletions(-)