[bug report] drm/msm/submit: Move copy_from_user ahead of locking bos

From: Dan Carpenter <dan.carpenter@oracle.com>
To: robdclark@chromium.org
Cc: linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org
Subject: [bug report] drm/msm/submit: Move copy_from_user ahead of locking bos
Date: Fri, 1 Oct 2021 17:26:28 +0300	[thread overview]
Message-ID: <20211001142628.GA11810@kili> (raw)

Hello Rob Clark,

The patch 20224d715a88: "drm/msm/submit: Move copy_from_user ahead of
locking bos" from Oct 23, 2020, leads to the following
Smatch static checker warning:

	drivers/gpu/drm/msm/msm_gem_submit.c:207 submit_lookup_cmds()
	warn: impossible condition '(sz == (~0)) => (0-u32max == u64max)'

drivers/gpu/drm/msm/msm_gem_submit.c
    161 static int submit_lookup_cmds(struct msm_gem_submit *submit,
    162                 struct drm_msm_gem_submit *args, struct drm_file *file)
    163 {
    164         unsigned i, sz;
    165         int ret = 0;
    166 
    167         for (i = 0; i < args->nr_cmds; i++) {
    168                 struct drm_msm_gem_submit_cmd submit_cmd;
    169                 void __user *userptr =
    170                         u64_to_user_ptr(args->cmds + (i * sizeof(submit_cmd)));
    171 
    172                 ret = copy_from_user(&submit_cmd, userptr, sizeof(submit_cmd));
    173                 if (ret) {
    174                         ret = -EFAULT;
    175                         goto out;
    176                 }
    177 
    178                 /* validate input from userspace: */
    179                 switch (submit_cmd.type) {
    180                 case MSM_SUBMIT_CMD_BUF:
    181                 case MSM_SUBMIT_CMD_IB_TARGET_BUF:
    182                 case MSM_SUBMIT_CMD_CTX_RESTORE_BUF:
    183                         break;
    184                 default:
    185                         DRM_ERROR("invalid type: %08x\n", submit_cmd.type);
    186                         return -EINVAL;
    187                 }
    188 
    189                 if (submit_cmd.size % 4) {
    190                         DRM_ERROR("non-aligned cmdstream buffer size: %u\n",
    191                                         submit_cmd.size);
    192                         ret = -EINVAL;
    193                         goto out;
    194                 }
    195 
    196                 submit->cmd[i].type = submit_cmd.type;
    197                 submit->cmd[i].size = submit_cmd.size / 4;
    198                 submit->cmd[i].offset = submit_cmd.submit_offset / 4;
    199                 submit->cmd[i].idx  = submit_cmd.submit_idx;
    200                 submit->cmd[i].nr_relocs = submit_cmd.nr_relocs;
    201 
    202                 userptr = u64_to_user_ptr(submit_cmd.relocs);
    203 
    204                 sz = array_size(submit_cmd.nr_relocs,
    205                                 sizeof(struct drm_msm_gem_submit_reloc));
    206                 /* check for overflow: */
--> 207                 if (sz == SIZE_MAX) {

"sz" is u32 (not size_t) so this check is impossible on 64 bit systems.
May as well just remove it and let the kmalloc() fail.  Or use a smaller
limit so that users can't trigger the kmalloc() failure.

    208                         ret = -ENOMEM;
    209                         goto out;
    210                 }
    211                 submit->cmd[i].relocs = kmalloc(sz, GFP_KERNEL);
    212                 ret = copy_from_user(submit->cmd[i].relocs, userptr, sz);
    213                 if (ret) {
    214                         ret = -EFAULT;
    215                         goto out;
    216                 }
    217         }
    218 
    219 out:
    220         return ret;
    221 }

regards,
dan carpenter