Make the following changes to CHANGE_HANDLE - Changing a non-existent handle to itself is ENOENT - idr_preload before idr_alloc - reject new_handle = 0 with EINVAL
This patch will not be merged until the relevant igt-tests (https://gitlab.freedesktop.org/fdavid-amd/igt-gpu-tools) are reviewed, merged, and run to the satisfaction of everyone involved. cc: Tvrtko Ursulin <[email protected]> cc: Christian Koenig <[email protected]> cc: Simona Vetter <[email protected]> Signed-off-by: David Francis <[email protected]> --- drivers/gpu/drm/drm_gem.c | 30 ++++++++++++++---------------- drivers/gpu/drm/drm_ioctl.c | 3 +-- 2 files changed, 15 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index e3ed684ddcf2..c8f3bf27b7de 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -1015,19 +1015,6 @@ drm_gem_open_ioctl(struct drm_device *dev, void *data, return ret; } -/* - * This ioctl is disabled for security reasons but also it failed - * to follow process in terms of adding testing in igt and verifying - * all the corner cases which made fixing security bugs in it even - * harder than necessary. - * - * To re-enable this ioctl - * 1. land working IGT tests in igt-gpu-tools that cover - * all corner cases and race conditions. - * 2. handle idr_preload - * 3. handle == 0 - * 4. handle == new_handle semantics definition. - */ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { @@ -1039,20 +1026,29 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data, return -EOPNOTSUPP; /* idr_alloc() limitation. */ - if (args->new_handle > INT_MAX) + if (args->new_handle == 0 || args->new_handle > INT_MAX) return -EINVAL; new_handle = args->new_handle; - if (args->handle == new_handle) - return 0; + if (args->handle == new_handle) { + spin_lock(&file_priv->table_lock); + if (idr_find(&file_priv->object_idr, args->handle)) + ret = 0; + else + ret = -ENOENT; + spin_unlock(&file_priv->table_lock); + return ret; + } mutex_lock(&file_priv->prime.lock); + idr_preload(GFP_KERNEL); spin_lock(&file_priv->table_lock); ret = idr_alloc(&file_priv->object_idr, NULL, new_handle, new_handle + 1, GFP_NOWAIT); if (ret < 0) { spin_unlock(&file_priv->table_lock); + idr_preload_end(); goto out_unlock; } @@ -1060,10 +1056,12 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data, if (IS_ERR_OR_NULL(obj)) { idr_remove(&file_priv->object_idr, new_handle); spin_unlock(&file_priv->table_lock); + idr_preload_end(); ret = -ENOENT; goto out_unlock; } spin_unlock(&file_priv->table_lock); + idr_preload_end(); if (obj->dma_buf) { ret = drm_prime_add_buf_handle(&file_priv->prime, obj->dma_buf, diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index e2df4becce62..ff193155129e 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -660,8 +660,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_GEM_CLOSE, drm_gem_close_ioctl, DRM_RENDER_ALLOW), DRM_IOCTL_DEF(DRM_IOCTL_GEM_FLINK, drm_gem_flink_ioctl, DRM_AUTH), DRM_IOCTL_DEF(DRM_IOCTL_GEM_OPEN, drm_gem_open_ioctl, DRM_AUTH), - /* see drm_gem.c:drm_gem_change_handle_ioctl for why this is invalid */ - DRM_IOCTL_DEF(DRM_IOCTL_GEM_CHANGE_HANDLE, drm_invalid_op, DRM_RENDER_ALLOW), + DRM_IOCTL_DEF(DRM_IOCTL_GEM_CHANGE_HANDLE, drm_gem_change_handle_ioctl, DRM_RENDER_ALLOW), DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETRESOURCES, drm_mode_getresources, 0), -- 2.34.1
