[bug report] drm/msm: Fix submit error-path leaks

[Dan Carpenter]

Hello Rob Clark,

Commit 68dc6c2d5eec ("drm/msm: Fix submit error-path leaks") from May
9, 2023 (linux-next), leads to the following Smatch static checker
warning:

drivers/gpu/drm/msm/msm_gem_submit.c:816 msm_ioctl_gem_submit() warn: fd used after fd_install() 'out_fence_fd'
drivers/gpu/drm/msm/msm_gem_submit.c:818 msm_ioctl_gem_submit() warn: fd used after fd_install() 'sync_file->file'

drivers/gpu/drm/msm/msm_gem_submit.c
    751                 WARN_ON(ret);
    752         } else {
    753                 /*
    754                  * Allocate an id which can be used by WAIT_FENCE ioctl to map
    755                  * back to the underlying fence.
    756                  */
    757                 submit->fence_id = idr_alloc_cyclic(&queue->fence_idr,
    758                                                     submit->user_fence, 1,
    759                                                     INT_MAX, GFP_NOWAIT);
    760         }
    761 
    762         spin_unlock(&queue->idr_lock);
    763         idr_preload_end();
    764 
    765         if (submit->fence_id < 0) {
    766                 ret = submit->fence_id;
    767                 submit->fence_id = 0;
    768         }
    769 
    770         if (ret == 0 && args->flags & MSM_SUBMIT_FENCE_FD_OUT) {
    771                 sync_file = sync_file_create(submit->user_fence);
    772                 if (!sync_file) {
    773                         ret = -ENOMEM;
    774                 } else {
    775                         fd_install(out_fence_fd, sync_file->file);
                                           ^^^^^^^^^^^^
Once we call fd_install() the file is exposed to userspace and they can make
the fd point to a different file.

    776                         args->fence_fd = out_fence_fd;
    777                 }
    778         }
    779 
    780         if (ret)
    781                 goto out;
    782 
    783         submit_attach_object_fences(submit);
    784 
    785         if (msm_context_is_vmbind(ctx)) {
    786                 /*
    787                  * If we are not using VM_BIND, submit_pin_vmas() will validate
    788                  * just the BOs attached to the submit.  In that case we don't
    789                  * need to validate the _entire_ vm, because userspace tracked
    790                  * what BOs are associated with the submit.
    791                  */
    792                 ret = drm_gpuvm_validate(submit->vm, &submit->exec);
    793                 if (ret)
    794                         goto out;
    795         }
    796 
    797         /* The scheduler owns a ref now: */
    798         msm_gem_submit_get(submit);
    799 
    800         msm_rd_dump_submit(priv->rd, submit, NULL);
    801 
    802         drm_sched_entity_push_job(&submit->base);
    803 
    804         args->fence = submit->fence_id;
    805         queue->last_fence = submit->fence_id;
    806 
    807         msm_syncobj_reset(syncobjs_to_reset, args->nr_in_syncobjs);
    808         msm_syncobj_process_post_deps(post_deps, args->nr_out_syncobjs, submit->user_fence);
    809 
    810 out:
    811         submit_cleanup(submit, !!ret);
    812 out_unlock:
    813         mutex_unlock(&queue->lock);
    814 out_post_unlock:
    815         if (ret && (out_fence_fd >= 0)) {
--> 816                 put_unused_fd(out_fence_fd);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
So this put_unused_fd() could potentially do something to the wrong file.
Traditionally, we either do the fd_install() last or we just leak until the
process dies and all the files are released.

(Hand wavey because I'm not sure how all this works exactly)

    817                 if (sync_file)
    818                         fput(sync_file->file);
    819         }
    820 
    821         if (!IS_ERR_OR_NULL(submit)) {
    822                 msm_gem_submit_put(submit);
    823         } else {
    824                 /*
    825                  * If the submit hasn't yet taken ownership of the queue
    826                  * then we need to drop the reference ourself:
    827                  */
    828                 msm_submitqueue_put(queue);
    829         }
    830         if (!IS_ERR_OR_NULL(post_deps)) {
    831                 for (i = 0; i < args->nr_out_syncobjs; ++i) {
    832                         kfree(post_deps[i].chain);
    833                         drm_syncobj_put(post_deps[i].syncobj);
    834                 }
    835                 kfree(post_deps);
    836         }
    837 
    838         if (!IS_ERR_OR_NULL(syncobjs_to_reset)) {
    839                 for (i = 0; i < args->nr_in_syncobjs; ++i) {
    840                         if (syncobjs_to_reset[i])
    841                                 drm_syncobj_put(syncobjs_to_reset[i]);
    842                 }
    843                 kfree(syncobjs_to_reset);
    844         }
    845 
    846         return ret;
    847 }

regards,
dan carpenter