BUG: KASAN: use-after-free in drm_gem_open_ioctl
There is potential for use-after-free here if the GEM object handle is closed between the idr lookup and retrieving the size from the object since a local reference is not being held at that point. Hold the local reference while the object can still be accessed to resolve this.
Signed-off-by: Steve Cohen cohens@codeaurora.org --- drivers/gpu/drm/drm_gem.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 7bf628e..4b2891c 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -898,14 +898,15 @@ drm_gem_open_ioctl(struct drm_device *dev, void *data,
/* drm_gem_handle_create_tail unlocks dev->object_name_lock. */ ret = drm_gem_handle_create_tail(file_priv, obj, &handle); - drm_gem_object_put_unlocked(obj); if (ret) - return ret; + goto out;
args->handle = handle; args->size = obj->size;
- return 0; +out: + drm_gem_object_put_unlocked(obj); + return ret; }
/**
Hi Steve and others.
On Fri, Jul 10, 2020 at 06:40:26PM -0400, Steve Cohen wrote:
BUG: KASAN: use-after-free in drm_gem_open_ioctl
There is potential for use-after-free here if the GEM object handle is closed between the idr lookup and retrieving the size from the object since a local reference is not being held at that point. Hold the local reference while the object can still be accessed to resolve this.
Signed-off-by: Steve Cohen cohens@codeaurora.org
drivers/gpu/drm/drm_gem.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 7bf628e..4b2891c 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -898,14 +898,15 @@ drm_gem_open_ioctl(struct drm_device *dev, void *data,
/* drm_gem_handle_create_tail unlocks dev->object_name_lock. */ ret = drm_gem_handle_create_tail(file_priv, obj, &handle);
- drm_gem_object_put_unlocked(obj); if (ret)
return ret;
goto out;args->handle = handle; args->size = obj->size;
- return 0;
+out:
- drm_gem_object_put_unlocked(obj);
- return ret;
Lookign at drm_gem_flink_ioctl() that is implmented just above this functions there are two things that I noted.
1) In drm_gem_flink_ioctl() the label is named "err:" - and my OCD likes that similar labels have the same name.
2) The function takes the object_name_lock but fails to release it in the error situation.
Danile Vetter updated the locking in 20228c447846da9399ead53fdbbc8ab69b47788a ("drm/gem: completely close gem_open vs. gem_close races")
but I failed to follow it all.
Sam
}
/**
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Steve.
On Thu, Jul 16, 2020 at 10:29:52PM +0200, Sam Ravnborg wrote:
Hi Steve and others.
On Fri, Jul 10, 2020 at 06:40:26PM -0400, Steve Cohen wrote:
BUG: KASAN: use-after-free in drm_gem_open_ioctl
There is potential for use-after-free here if the GEM object handle is closed between the idr lookup and retrieving the size from the object since a local reference is not being held at that point. Hold the local reference while the object can still be accessed to resolve this.
Signed-off-by: Steve Cohen cohens@codeaurora.org
drivers/gpu/drm/drm_gem.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 7bf628e..4b2891c 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -898,14 +898,15 @@ drm_gem_open_ioctl(struct drm_device *dev, void *data,
/* drm_gem_handle_create_tail unlocks dev->object_name_lock. */ ret = drm_gem_handle_create_tail(file_priv, obj, &handle);
- drm_gem_object_put_unlocked(obj); if (ret)
return ret;
goto out;args->handle = handle; args->size = obj->size;
- return 0;
+out:
- drm_gem_object_put_unlocked(obj);
- return ret;
Lookign at drm_gem_flink_ioctl() that is implmented just above this functions there are two things that I noted.
- In drm_gem_flink_ioctl() the label is named "err:" - and my OCD likes
that similar labels have the same name.
- The function takes the object_name_lock but fails to release it in
the error situation.
Daniel pointed out on irc that drm_gem_handle_create_tail releases the lock. If I had read the comment I would have noticed too - sigh.
With the label name fixed to "err:" like used in the function above: Reviewed-by: Sam Ravnborg sam@ravnborg.org
Please re-submit.
Sam
Danile Vetter updated the locking in 20228c447846da9399ead53fdbbc8ab69b47788a ("drm/gem: completely close gem_open vs. gem_close races")
but I failed to follow it all.
Sam
}
/**
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Sam Ravnborg -
Steve Cohen