flink_to is similar to flink, but the global name is only made available to one other drm fd, identified by the existing drm_magic_t cookie mechanism.
flink_to names are transient. Once the receiving fd opens a name, it goes away. flink_to lets application attach a binary blob of data to the name, which is passed to the receiving fd when it uses the open_with_data ioctl. This lets us pass meta data about the buffer along with the name, which generalizes how we currently passes object size, tile and swizzle information.
With the flink_to ioctl, the X server can specify exactly which clients should be able to access the window buffers, which avoids leaking buffer access to other X servers or unpriviledged X clients.
Signed-off-by: Kristian Høgsberg krh@bitplanet.net ---
We've discussed how the current drm security model is broken in different ways before. This is my proposal for fixing it:
- drm auth today is a bit in the file_priv, set by the drm master when a client requests it (typically through dri2 protocol). Doesn't take multiple masters or fine-grained buffers access into account.
- broken in multiple masters world; vt switch, server drops master, then anybody can become master and auth themselves. client is now authenticated and can access all buffers from the X server.
- XACE, no way to restrict access to window buffers, once one client is direct rendering to the window, all authenticated drm clients can access the buffers.
What is flink_to
- similar to flink, but global name is only made available to one other drm fd, identified by the existing drm_magic_t cookie mechanism.
- flink_to names are transient, once the receiving fd opens it, it goes away. makes user space easier, we don't have to track "did we already flink_to this buffer to that client and what was the name".
- flink_to fails if the receiving fd already has an flink_to name pending (EBUSY). this prevents unlimited flink_to names from piling up if the receiving fd doesn't open them.
- flink_to twice on the same bo gives different names.
- flink_to lets application attach a binary blob of data to the name (see below).
How does flink_to fix the security problems
- for dri2, the X server will use flink_to to grant each client access to the dri2 buffers as they call dri2 getbuffers on a per buffer basis.
- drm applications that aren't X clients can't talk to dri2 and can't get access. vt switching doesn't affect this in any way.
- xace can reject individual clients access to the dri2 extension, preventing those applications from accessing window buffers they aren't authorized for.
Suggest to drop auth requirement for gem ioctls
- rely on /dev/dri/card0 file permissions to restrict access to gpu to local users. similar to how it works for most other hw.
- access to buffers is in the drm-with-mm world is what needs to be controlled.
- keep auth and master requirement for kms and old drm ioctls
- allows gpgpu type applications
- risks: you now don't need to be an X client to access the gpu, malicious applications can starve or maybe crash gpu. malicious programs submitting hand-crafted batch buffers to read from absolute addresses? X front buffer location in gtt is pretty predictable. needs cmd checker or ppgtt or such to be 100% secure.
Attached data
- a mechanism to attach a binary blob to an flink_to buffer name. open_with_data returns the data. Userspace (typically libdrm) decides the layout and versioning of the blob and the contents will be chipset specific. it's an opaque blob to the kernel, which doesn't need to know about stride and formats etc.
- applications use this to pass metadata about the buffer along with the buffer name. this keeps chipset specific details out of ipc. We already share object size, tiling and swizzling information through the kernel and need to ioctl immediately after gem_open to get those details. this mechanism lets us pass that info along with other meta data and return it all as we open the buffer with open_with_data.
- EGLImage sharing: assign a global name to an EGLImage to share with a different process. Used by wayland or potentially other non-X systems. Khronos EGL extension in the works.
Backwards compat
- an flink_to name can be opened by old gem open, which succeeds if it was flinked_to magic 0 or the file_priv of the opener. If the name was flinked to magic 0, the name is not transient (it's not reclaimed by opening it). The X server can then use flink_to in getbuffers, and old and new clients can still use plain gem_open to open the buffers. This requires that clients only gem_open a name once per getbuffers request. this is currently true for all dri2 drivers.
- Or could just introduce new getbuffers request that returns flink_to names for all buffers and no metadata (require the ddx driver to attach the meta data to the name). this new request will also make it explicit that a name can only be opened once and that the driver must open all received names.
Oops, sorry, this got way too long...
Kristian
drivers/gpu/drm/drm_drv.c | 2 + drivers/gpu/drm/drm_gem.c | 224 +++++++++++++++++++++++++---------- drivers/gpu/drm/drm_info.c | 4 +- drivers/gpu/drm/i915/i915_debugfs.c | 26 ++++- drivers/gpu/drm/i915/i915_gem.c | 2 +- drivers/gpu/drm/i915/i915_irq.c | 2 +- include/drm/drm.h | 35 ++++++ include/drm/drmP.h | 20 +++- 8 files changed, 243 insertions(+), 72 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 4a66201..ee58a9e 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -129,6 +129,8 @@ static struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_GEM_CLOSE, drm_gem_close_ioctl, DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_GEM_FLINK, drm_gem_flink_ioctl, DRM_AUTH|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_GEM_OPEN, drm_gem_open_ioctl, DRM_AUTH|DRM_UNLOCKED), + DRM_IOCTL_DEF(DRM_IOCTL_GEM_FLINK_TO, drm_gem_flink_to_ioctl, DRM_AUTH|DRM_UNLOCKED), + DRM_IOCTL_DEF(DRM_IOCTL_GEM_OPEN_WITH_DATA, drm_gem_open_with_data_ioctl, DRM_AUTH|DRM_UNLOCKED),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETRESOURCES, drm_mode_getresources, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 33dad3f..8f8fd9f 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -140,6 +140,7 @@ int drm_gem_object_init(struct drm_device *dev, kref_init(&obj->refcount); kref_init(&obj->handlecount); obj->size = size; + INIT_LIST_HEAD(&obj->name_list);
atomic_inc(&dev->object_count); atomic_add(obj->size, &dev->object_memory); @@ -293,27 +294,45 @@ drm_gem_close_ioctl(struct drm_device *dev, void *data, return ret; }
-/** - * Create a global name for an object, returning the name. - * - * Note that the name does not hold a reference; when the object - * is freed, the name goes away. - */ int -drm_gem_flink_ioctl(struct drm_device *dev, void *data, - struct drm_file *file_priv) +drm_gem_flink_to_ioctl(struct drm_device *dev, void *data, + struct drm_file *file_priv) { - struct drm_gem_flink *args = data; + struct drm_gem_flink_to *args = data; + struct drm_gem_name *name, *n; struct drm_gem_object *obj; + void __user *udata; int ret;
if (!(dev->driver->driver_features & DRIVER_GEM)) return -ENODEV;
+ if (args->data_size > 2048) + return -EINVAL; + + if (args->magic == 0 && args->data_size > 0) + return -EINVAL; + obj = drm_gem_object_lookup(dev, file_priv, args->handle); if (obj == NULL) return -EBADF;
+ name = kmalloc(sizeof *name + args->data_size, GFP_KERNEL); + if (name == NULL) { + ret = -ENOMEM; + goto err; + } + + name->obj = obj; + name->magic = args->magic; + name->data_size = args->data_size; + + udata = (void __user *) (long) args->data; + if (copy_from_user(name->data, udata, args->data_size)) { + ret = -EFAULT; + goto err; + } + again: if (idr_pre_get(&dev->object_name_idr, GFP_KERNEL) == 0) { ret = -ENOMEM; @@ -321,31 +340,99 @@ again: }
spin_lock(&dev->object_name_lock); - if (!obj->name) { - ret = idr_get_new_above(&dev->object_name_idr, obj, 1, - &obj->name); - args->name = (uint64_t) obj->name; - spin_unlock(&dev->object_name_lock); - - if (ret == -EAGAIN) - goto again; - - if (ret != 0) - goto err; - - /* Allocate a reference for the name table. */ - drm_gem_object_reference(obj); - } else { - args->name = (uint64_t) obj->name; - spin_unlock(&dev->object_name_lock); - ret = 0; + + /* + * Find out if there already is a name for the given magic for + * this object. If magic is 0, then we're done and will + * return that name, but if magic is not 0, it's a bug and we + * return -EBUSY. Otherwise fall through and allocate a new + * name in the idr. + */ + list_for_each_entry(n, &obj->name_list, list) { + if (n->magic == 0 && args->magic == 0) { + args->name = n->name; + ret = 0; + goto err_lock; + } else if (n->magic == args->magic) { + ret = -EBUSY; + goto err_lock; + } }
+ ret = idr_get_new_above(&dev->object_name_idr, obj, 1, &name->name); + args->name = name->name; + if (ret == 0) + list_add_tail(&name->list, &obj->name_list); + + spin_unlock(&dev->object_name_lock); + + if (ret == -EAGAIN) + goto again; + + if (ret != 0) + goto err; + + /* Keep the reference for the name object. */ + + return 0; + +err_lock: + spin_unlock(&dev->object_name_lock); err: + kfree(name); drm_gem_object_unreference_unlocked(obj); return ret; }
+int +drm_gem_open_with_data_ioctl(struct drm_device *dev, void *data, + struct drm_file *file_priv) +{ + struct drm_gem_open_with_data *args = data; + struct drm_gem_name *name; + void __user *udata; + int ret = 0; + + if (!(dev->driver->driver_features & DRIVER_GEM)) + return -ENODEV; + + spin_lock(&dev->object_name_lock); + name = idr_find(&dev->object_name_idr, (int) args->name); + if (name && file_priv->magic == name->magic) + list_del(&name->list); + if (name && name->magic != 0) + name = NULL; + spin_unlock(&dev->object_name_lock); + if (!name) + return -ENOENT; + + /* + * We now have a name object for either the global (magic 0) + * name which stays around as long as the bo, or an name + * object that we just took out of the list and will free + * below. In other words, we know we can acess it outside the + * object_name_lock. + */ + + udata = (void __user *) (unsigned long) args->data; + if (copy_to_user(udata, name->data, + min(args->data_size, name->data_size))) { + ret = -EFAULT; + goto err; + } + + ret = drm_gem_handle_create(file_priv, name->obj, &args->handle); + args->data_size = name->data_size; + + err: + if (name->magic != 0) { + drm_gem_object_unreference_unlocked(name->obj); + kfree(name); + } + + return ret; +} + /** * Open an object using the global name, returning a handle and the size. * @@ -357,30 +444,52 @@ drm_gem_open_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_gem_open *args = data; + struct drm_gem_open_with_data open_with_data; struct drm_gem_object *obj; int ret; - u32 handle;
- if (!(dev->driver->driver_features & DRIVER_GEM)) - return -ENODEV; + open_with_data.name = args->name; + open_with_data.data = 0; + open_with_data.data_size = 0;
- spin_lock(&dev->object_name_lock); - obj = idr_find(&dev->object_name_idr, (int) args->name); - if (obj) - drm_gem_object_reference(obj); - spin_unlock(&dev->object_name_lock); - if (!obj) - return -ENOENT; + ret = drm_gem_open_with_data_ioctl(dev, &open_with_data, file_priv);
- ret = drm_gem_handle_create(file_priv, obj, &handle); - drm_gem_object_unreference_unlocked(obj); - if (ret) - return ret; + if (ret == 0) { + args->handle = open_with_data.handle; + obj = drm_gem_object_lookup(dev, file_priv, args->handle); + BUG_ON(!obj); + args->size = obj->size; + drm_gem_object_unreference_unlocked(obj); + }
- args->handle = handle; - args->size = obj->size; + return ret; +}
- return 0; +/** + * Create a global name for an object, returning the name. + * + * Note that the name does not hold a reference; when the object + * is freed, the name goes away. + */ +int +drm_gem_flink_ioctl(struct drm_device *dev, void *data, + struct drm_file *file_priv) +{ + struct drm_gem_flink *args = data; + struct drm_gem_flink_to flink_to; + int ret; + + flink_to.handle = args->handle; + flink_to.magic = 0; + flink_to.data = 0; + flink_to.data_size = 0; + + ret = drm_gem_flink_to_ioctl(dev, &flink_to, file_priv); + + if (ret == 0) + args->name = flink_to.name; + + return ret; }
/** @@ -488,27 +597,20 @@ static void drm_gem_object_ref_bug(struct kref *list_kref) void drm_gem_object_handle_free(struct kref *kref) { - struct drm_gem_object *obj = container_of(kref, - struct drm_gem_object, - handlecount); + struct drm_gem_object *obj = + container_of(kref, struct drm_gem_object, handlecount); struct drm_device *dev = obj->dev; + struct drm_gem_name *n, *tmp;
- /* Remove any name for this object */ + /* Remove all names for this object */ spin_lock(&dev->object_name_lock); - if (obj->name) { - idr_remove(&dev->object_name_idr, obj->name); - obj->name = 0; - spin_unlock(&dev->object_name_lock); - /* - * The object name held a reference to this object, drop - * that now. - * - * This cannot be the last reference, since the handle holds one too. - */ + list_for_each_entry_safe(n, tmp, &obj->name_list, list) { + idr_remove(&dev->object_name_idr, n->name); kref_put(&obj->refcount, drm_gem_object_ref_bug); - } else - spin_unlock(&dev->object_name_lock); - + list_del(&n->list); + kfree(n); + } + spin_unlock(&dev->object_name_lock); } EXPORT_SYMBOL(drm_gem_object_handle_free);
diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c index f0f6c6b..9abe00d 100644 --- a/drivers/gpu/drm/drm_info.c +++ b/drivers/gpu/drm/drm_info.c @@ -240,10 +240,10 @@ int drm_gem_one_name_info(int id, void *ptr, void *data) struct drm_gem_object *obj = ptr; struct seq_file *m = data;
- seq_printf(m, "name %d size %zd\n", obj->name, obj->size); + seq_printf(m, "name %d size %zd\n", id, obj->size);
seq_printf(m, "%6d %8zd %7d %8d\n", - obj->name, obj->size, + id, obj->size, atomic_read(&obj->handlecount.refcount), atomic_read(&obj->refcount.refcount)); return 0; diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 52510ad..cbc1851 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -63,6 +63,25 @@ static const char *get_tiling_flag(struct drm_i915_gem_object *obj_priv) } }
+ +static void print_object_names(struct seq_file *m, + struct drm_device *dev, + struct drm_gem_object *obj) +{ + struct drm_gem_name *n; + + spin_lock(&dev->object_name_lock); + + if (!list_empty(&obj->name_list)) { + seq_printf(m, " (names:"); + list_for_each_entry(n, &obj->name_list, list) + seq_printf(m, " %d", n->name); + seq_printf(m, ")"); + } + + spin_unlock(&dev->object_name_lock); +} + static int i915_gem_object_list_info(struct seq_file *m, void *data) { struct drm_info_node *node = (struct drm_info_node *) m->private; @@ -106,8 +125,8 @@ static int i915_gem_object_list_info(struct seq_file *m, void *data) obj_priv->dirty ? " dirty" : "", obj_priv->madv == I915_MADV_DONTNEED ? " purgeable" : "");
- if (obj_priv->base.name) - seq_printf(m, " (name: %d)", obj_priv->base.name); + print_object_names(m, dev, &obj_priv->base); + if (obj_priv->fence_reg != I915_FENCE_REG_NONE) seq_printf(m, " (fence: %d)", obj_priv->fence_reg); if (obj_priv->gtt_space != NULL) @@ -235,8 +254,7 @@ static int i915_gem_fence_regs_info(struct seq_file *m, void *data) get_tiling_flag(obj_priv), obj->read_domains, obj->write_domain, obj_priv->last_rendering_seqno); - if (obj->name) - seq_printf(m, " (name: %d)", obj->name); + print_object_names(m, dev, obj); seq_printf(m, "\n"); } } diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 9ded3da..dac0f6d 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1225,7 +1225,7 @@ i915_gem_create_mmap_offset(struct drm_gem_object *obj) list->file_offset_node = drm_mm_search_free(&mm->offset_manager, obj->size / PAGE_SIZE, 0, 0); if (!list->file_offset_node) { - DRM_ERROR("failed to allocate offset for bo %d\n", obj->name); + DRM_ERROR("failed to allocate offset for bo %p\n", obj); ret = -ENOMEM; goto out_free_list; } diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 2479be0..cfcca51 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -644,7 +644,7 @@ static void i915_capture_error_state(struct drm_device *dev) struct drm_gem_object *obj = &obj_priv->base;
error->active_bo[i].size = obj->size; - error->active_bo[i].name = obj->name; + error->active_bo[i].name = 0; error->active_bo[i].seqno = obj_priv->last_rendering_seqno; error->active_bo[i].gtt_offset = obj_priv->gtt_offset; error->active_bo[i].read_domains = obj->read_domains; diff --git a/include/drm/drm.h b/include/drm/drm.h index e3f46e0..cb3f938 100644 --- a/include/drm/drm.h +++ b/include/drm/drm.h @@ -608,6 +608,39 @@ struct drm_gem_open { __u64 size; };
+struct drm_gem_flink_to { + /** Handle for the object being named */ + __u32 handle; + + /** Magic for destination file_priv */ + __u32 magic; + + /** Data to attach to name */ + __u64 data; + + /** Size of data, max is 2k bytes */ + __u32 data_size; + + /** Returned name for the object */ + __u32 name; +}; + +struct drm_gem_open_with_data { + /** Name of object being opened */ + __u32 name; + + /** Returned handle for the object */ + __u32 handle; + + /** Pointer to userspace buffer for data */ + __u64 data; + + /** Size of userspace buffer, returned size of the object */ + __u32 data_size; + + __u32 pad; +}; + #include "drm_mode.h"
#define DRM_IOCTL_BASE 'd' @@ -628,6 +661,8 @@ struct drm_gem_open { #define DRM_IOCTL_GEM_CLOSE DRM_IOW (0x09, struct drm_gem_close) #define DRM_IOCTL_GEM_FLINK DRM_IOWR(0x0a, struct drm_gem_flink) #define DRM_IOCTL_GEM_OPEN DRM_IOWR(0x0b, struct drm_gem_open) +#define DRM_IOCTL_GEM_FLINK_TO DRM_IOWR(0x0c, struct drm_gem_flink_to) +#define DRM_IOCTL_GEM_OPEN_WITH_DATA DRM_IOWR(0x0d, struct drm_gem_open_with_data)
#define DRM_IOCTL_SET_UNIQUE DRM_IOW( 0x10, struct drm_unique) #define DRM_IOCTL_AUTH_MAGIC DRM_IOW( 0x11, struct drm_auth) diff --git a/include/drm/drmP.h b/include/drm/drmP.h index c1b9871..f60b35c 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -597,6 +597,15 @@ struct drm_gem_mm { struct drm_open_hash offset_hash; /**< User token hash table for maps */ };
+struct drm_gem_name { + struct drm_gem_object *obj; + drm_magic_t magic; + struct list_head list; + u32 name; + u32 data_size; + char data[0]; +}; + /** * This structure defines the drm_mm memory object, which will be used by the * DRM for its buffer objects. @@ -624,10 +633,11 @@ struct drm_gem_object { size_t size;
/** - * Global name for this object, starts at 1. 0 means unnamed. - * Access is covered by the object_name_lock in the related drm_device + * Global names for this object, starts at 1. Empty list means + * unnamed. Access is covered by the object_name_lock in the + * related drm_device */ - int name; + struct list_head name_list;
/** * Memory domains. These monitor which caches contain read/write data @@ -1510,6 +1520,10 @@ int drm_gem_flink_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); int drm_gem_open_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); +int drm_gem_flink_to_ioctl(struct drm_device *dev, void *data, + struct drm_file *file_priv); +int drm_gem_open_with_data_ioctl(struct drm_device *dev, void *data, + struct drm_file *file_priv); void drm_gem_open(struct drm_device *dev, struct drm_file *file_private); void drm_gem_release(struct drm_device *dev, struct drm_file *file_private);