Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [Critical] Direct access to `ctx->vm` without calling `msm_context_vm()` leads to a NULL pointer dereference in `msm_gem_new_handle()`. - [Critical] Adding to the saturated result of `struct_size()` causes an integer wrap-around on overflow, defeating overflow protection and leading to a massive heap buffer overflow. - [High] Validation bypass in `adreno_set_param()` when enabling `MSM_PARAM_EN_VM_BIND` allows userspace to enable VM BIND on unsupported hardware, leading to a system-wide GPU DoS. -- --- Patch [3]: [PATCH v4 3/6] drm/msm: Validate lazy VM is created in GEM_SUBMIT --- commit a317c04b26f6bc5a511792869e55bcddc1d9014f Author: Rob Clark <[email protected]> drm/msm: Validate lazy VM is created in GEM_SUBMIT This commit fixes a potential NULL pointer dereference in the GEM_SUBMIT ioctl by ensuring the lazy VM is created before validating it. It passes the instantiated VM to submit_create() to avoid redundant context lookups. > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c > b/drivers/gpu/drm/msm/msm_gem_submit.c > index 26ea8a28be474..0dc983d155cd2 100644 > --- a/drivers/gpu/drm/msm/msm_gem_submit.c > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c > @@ -30,7 +30,7 @@ > */ > > static struct msm_gem_submit *submit_create(struct drm_device *dev, > - struct msm_gpu *gpu, > + struct msm_gpu *gpu, struct drm_gpuvm *vm, > struct msm_gpu_submitqueue *queue, uint32_t nr_bos, > uint32_t nr_cmds, u64 drm_client_id) > { [Severity: Critical] This is a pre-existing issue, but can this size calculation overflow? submit_create() calculates the size as: sz = struct_size(submit, bos, nr_bos) + ((u64)nr_cmds * sizeof(submit->cmd[0])); if (sz > SIZE_MAX) return ERR_PTR(-ENOMEM); If struct_size() saturates to SIZE_MAX, adding to it will wrap around uint64_t. Also, the sz > SIZE_MAX check is dead code on 64-bit systems because an unsigned 64-bit integer can never exceed SIZE_MAX. This defeats overflow protection and could allow an undersized allocation to succeed, potentially leading to an out-of-bounds heap write when submit->bos and submit->cmd are processed. [ ... ] While reviewing this commit's handling of lazy VM initialization, I noticed two other pre-existing issues in related code where ctx->vm is accessed without being instantiated: [Severity: Critical] This is a pre-existing issue, but does msm_gem_new_handle() dereference a NULL pointer if the VM isn't created yet? In drivers/gpu/drm/msm/msm_gem.c:msm_gem_new_handle(): struct drm_gem_object *r_obj = drm_gpuvm_resv_obj(ctx->vm); Since ctx->vm is lazily initialized, it will be NULL if msm_context_vm() hasn't been called. drm_gpuvm_resv_obj expands to (ctx->vm)->r_obj, which would dereference NULL. Could an unprivileged userspace program trigger this by calling DRM_IOCTL_MSM_GEM_NEW with MSM_BO_NO_SHARE immediately after opening the device? [Severity: High] This is a pre-existing issue, but is the VM_BIND validation bypassed in adreno_set_param()? In drivers/gpu/drm/msm/adreno/adreno_gpu.c:adreno_set_param(): if (ctx->vm == gpu->vm) return UERR(EINVAL, drm, "requires per-process pgtables"); Because ctx->vm is lazily created, it is NULL here. This makes the check evaluate to NULL == gpu->vm, which is false, bypassing the validation. Could this allow userspace to enable VM BIND on unsupported hardware and cause lock contention during job submission? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
