Hi

On Thu, Jul 24, 2014 at 3:53 AM, Dave Airlie airlied@gmail.com wrote:

From: Dave Airlie airlied@redhat.com

Daniel pointed out with hotplug that userspace could be trying to oops us as root for lols, and that to be correct we shouldn't register the object with the idr before we have fully set the connector object up.

His proposed solution was a lot more life changing, this seemed like a simpler proposition to me, get the connector object id from the idr, but don't register the object until the drm_connector_register callback.

The open question is whether the drm_mode_object_register needs a bigger lock than just the idr one, but I can't see why it would, but I can be locking challenged.

Signed-off-by: Dave Airlie airlied@redhat.com

drivers/gpu/drm/drm_crtc.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 1ccf5cb..9f8cc1a 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -376,13 +376,14 @@ EXPORT_SYMBOL(drm_get_format_name);

• New unique (relative to other objects in @dev) integer identifier for the
• object.

*/ -int drm_mode_object_get(struct drm_device *dev,

•                   struct drm_mode_object *obj, uint32_t obj_type)
  

+static int drm_mode_object_get_noreg(struct drm_device *dev,

•                                struct drm_mode_object *obj, uint32_t obj_type,
  

•                                bool noreg)
  

{ int ret;

    mutex_lock(&dev->mode_config.idr_mutex);

•   ret = idr_alloc(&dev->mode_config.crtc_idr, obj, 1, 0, GFP_KERNEL);
  

•   ret = idr_alloc(&dev->mode_config.crtc_idr, noreg ? NULL : obj, 1, 0, GFP_KERNEL);
    if (ret >= 0) {
            /*
             * Set up the object linking under the protection of the idr
  

@@ -396,6 +397,20 @@ int drm_mode_object_get(struct drm_device *dev, return ret < 0 ? ret : 0; }

+int drm_mode_object_get(struct drm_device *dev,

•                   struct drm_mode_object *obj, uint32_t obj_type)
  

+{

•   return drm_mode_object_get_noreg(dev, obj, obj_type, false);
  

+}

+static void drm_mode_object_register(struct drm_device *dev,

•                                struct drm_mode_object *obj)
  

+{

•   mutex_lock(&dev->mode_config.idr_mutex);
  

•   idr_replace(&dev->mode_config.crtc_idr, obj, obj->id);
  

•   mutex_unlock(&dev->mode_config.idr_mutex);
  

+}

/**

• drm_mode_object_put - free a modeset identifer
• @dev: DRM device

@@ -849,7 +864,7 @@ int drm_connector_init(struct drm_device *dev,

    drm_modeset_lock_all(dev);

•   ret = drm_mode_object_get(dev, &connector->base, DRM_MODE_OBJECT_CONNECTOR);
  

•   ret = drm_mode_object_get_noreg(dev, &connector->base, DRM_MODE_OBJECT_CONNECTOR, true);
    if (ret)
            goto out_unlock;
  
  

@@ -942,6 +957,8 @@ int drm_connector_register(struct drm_connector *connector) { int ret;

•   drm_mode_object_register(connector->dev, &connector->base);
  

This is the same we do for minor-objects, so fine with me. I'd prefer if the registration is done last in drm_connector_register(), not first, but I'm not sure the debugfs hooks work without the connector available in the lookup tables. Maybe worth a try.

Maybe you also want to do the reverse in drm_connector_unregister()? Making sure no user-space call can look them up anymore.

This patch is: Reviewed-by: David Herrmann dh.herrmann@gmail.com

Thanks David

    ret = drm_sysfs_connector_add(connector);
    if (ret)
            return ret;

-- 1.9.3

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel