Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [Critical] `msm_context_vm()` propagates errors as `ERR_PTR` or `NULL` without callers checking, leading to wild pointer dereferences. - [Critical] Direct access to uninitialized `ctx->vm` in `msm_ioctl_gem_submit()` causes a guaranteed NULL pointer dereference. - [High] Hardware capability check in `adreno_set_param()` fails due to lazy VM initialization, bypassing restrictions. -- --- Patch [3]: [PATCH v6 03/16] drm/msm: Synchronize VM creation on ctxlock --- commit 78d3ed784b530d2c180edb9b49939bea521dccd3 Author: Rob Clark <[email protected]> drm/msm: Synchronize VM creation on ctxlock This commit serializes the setting of EN_VM_BIND against VM creation by synchronizing it on ctxlock. > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > index c62c45bb0ddbb..0d54141ff089c 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c > @@ -503,7 +503,9 @@ int adreno_set_param(struct msm_gpu *gpu, struct > msm_context *ctx, > if (!perfmon_capable()) > return UERR(EPERM, drm, "invalid permissions"); > return msm_context_set_sysprof(ctx, gpu, value); > - case MSM_PARAM_EN_VM_BIND: > + case MSM_PARAM_EN_VM_BIND: { > + guard(rwsem_read)(&ctx->ctxlock); > + > /* We can only support VM_BIND with per-process pgtables: */ > if (ctx->vm == gpu->vm) [Severity: High] This isn't a bug introduced by this patch, but does this hardware capability check properly account for lazy VM initialization? Since VMs are lazily initialized, ctx->vm will be NULL at this stage. This means the check evaluates NULL == gpu->vm, which is always false. Does this allow all hardware, even those without per-process pagetables, to bypass this restriction? > return UERR(EINVAL, drm, "requires per-process > pgtables"); [ ... ] > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index 380b3e2bf734e..20c5a6cc82019 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c > @@ -223,7 +223,6 @@ static void load_gpu(struct drm_device *dev) > */ > struct drm_gpuvm *msm_context_vm(struct drm_device *dev, struct msm_context > *ctx) > { > - static DEFINE_MUTEX(init_lock); > struct msm_drm_private *priv = dev->dev_private; > struct drm_gpuvm *vm = smp_load_acquire(&ctx->vm); > > @@ -231,7 +230,7 @@ struct drm_gpuvm *msm_context_vm(struct drm_device *dev, > struct msm_context *ctx > if (vm) > return vm; > > - guard(mutex)(&init_lock); > + guard(rwsem_write)(&ctx->ctxlock); > > if (!ctx->vm) { > vm = msm_gpu_create_private_vm( [Severity: Critical] This is a pre-existing issue, but does msm_context_vm() propagate an unhandled error pointer or NULL on failure? If msm_gpu_create_private_vm() returns ERR_PTR or NULL (e.g., during an OOM condition or on platforms without per-process pagetables), it appears msm_context_vm() stores and returns this directly. Callers like msm_submitqueue_create() in msm_submitqueue.c blindly assume success: struct drm_gpu_scheduler *sched = &to_msm_vm(msm_context_vm(drm, ctx))->sched; If an error pointer or NULL is returned, to_msm_vm() yields an invalid address. Could this lead to an immediate kernel crash when it is dereferenced in drm_sched_entity_init()? [Severity: Critical] This is another pre-existing issue, but does direct access to ctx->vm cause a NULL pointer dereference? If a user issues a DRM_IOCTL_MSM_GEM_SUBMIT ioctl before executing any commands that lazily initialize the VM, ctx->vm is still NULL. In msm_ioctl_gem_submit(): if (to_msm_vm(ctx->vm)->unusable) return UERR(EPIPE, dev, "context is unusable"); Because ctx->vm is NULL, the to_msm_vm() macro returns a negative offset pointer. Does dereferencing ->unusable here immediately crash the kernel instead of calling msm_context_vm() to lazily initialize it? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
