6.9-stable review patch. If anyone has any objections, please let me know.
------------------
From: Jann Horn jannh@google.com
commit 4f2a129b33a2054e62273edd5a051c34c08d96e9 upstream.
maarten.lankhorst@linux.intel.com, Maxime Ripard mripard@kernel.org, Thomas Zimmermann tzimmermann@suse.de
filp->pid is supposed to be a refcounted pointer; however, before this patch, drm_file_update_pid() only increments the refcount of a struct pid after storing a pointer to it in filp->pid and dropping the dev->filelist_mutex, making the following race possible:
process A process B ========= ========= begin drm_file_update_pid mutex_lock(&dev->filelist_mutex) rcu_replace_pointer(filp->pid, <pid B>, 1) mutex_unlock(&dev->filelist_mutex) begin drm_file_update_pid mutex_lock(&dev->filelist_mutex) rcu_replace_pointer(filp->pid, <pid A>, 1) mutex_unlock(&dev->filelist_mutex) get_pid(<pid A>) synchronize_rcu() put_pid(<pid B>) *** pid B reaches refcount 0 and is freed here *** get_pid(<pid B>) *** UAF *** synchronize_rcu() put_pid(<pid A>)
As far as I know, this race can only occur with CONFIG_PREEMPT_RCU=y because it requires RCU to detect a quiescent state in code that is not explicitly calling into the scheduler.
This race leads to use-after-free of a "struct pid". It is probably somewhat hard to hit because process A has to pass through a synchronize_rcu() operation while process B is between mutex_unlock() and get_pid().
Fix it by ensuring that by the time a pointer to the current task's pid is stored in the file, an extra reference to the pid has been taken.
This fix also removes the condition for synchronize_rcu(); I think that optimization is unnecessary complexity, since in that case we would usually have bailed out on the lockless check above.
Fixes: 1c7a387ffef8 ("drm: Update file owner during use") Cc: stable@vger.kernel.org Signed-off-by: Jann Horn jannh@google.com Signed-off-by: Dave Airlie airlied@redhat.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- drivers/gpu/drm/drm_file.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
--- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -469,14 +469,12 @@ void drm_file_update_pid(struct drm_file
dev = filp->minor->dev; mutex_lock(&dev->filelist_mutex); + get_pid(pid); old = rcu_replace_pointer(filp->pid, pid, 1); mutex_unlock(&dev->filelist_mutex);
- if (pid != old) { - get_pid(pid); - synchronize_rcu(); - put_pid(old); - } + synchronize_rcu(); + put_pid(old); }
/**