Re: [PATCH v7 03/17] drm/msm: Synchronize VM creation on ctxlock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] `msm_context_vm()` returns `NULL` when VM creation fails, leading to unhandled NULL pointer dereference in callers.
- [Critical] Flawed hardware capability check in `adreno_set_param()` allows VM_BIND on unsupported hardware.
- [High] Missing error check for `drm_sched_entity_init()` leaves the scheduler entity uninitialized.
--

--- Patch [3]: [PATCH v7 03/17] drm/msm: Synchronize VM creation on ctxlock ---
commit fd449ed84ba9ef6ab17d7a557f7bc86f9a0a680e
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Synchronize VM creation on ctxlock

This commit replaces the global init_lock mutex with a per-context
rw_semaphore to synchronize VM creation and serialize setting EN_VM_BIND.

> 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)
> -			return UERR(EINVAL, drm, "requires per-process pgtables");

[Severity: Critical]
This isn't a bug introduced by this patch, but does this hardware capability
check evaluate correctly before VM creation?

Before this change, the check if (ctx->vm == gpu->vm) evaluates to false
because ctx->vm is NULL before lazy VM creation. This allows an unprivileged
user to successfully set ctx->userspace_managed_vm on hardware lacking
create_private_vm support. 

When the VM is subsequently created, it returns NULL, and the caller crashes.

[ ... ]
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 77681cb4fdc25..a5714e24fcc6c 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
[ ... ]
> @@ -230,7 +229,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 isn't a bug introduced by this patch, but does msm_context_vm()
properly propagate errors if VM creation fails?

If msm_gpu_create_private_vm() fails (e.g., due to memory exhaustion) and
returns an error pointer, msm_context_vm() does not populate ctx->vm and
returns NULL.

Callers like msm_submitqueue_create() unconditionally cast the return value
using to_msm_vm() and dereference its ->sched member, which can cause a
kernel panic.

I also noticed a pre-existing issue in msm_submitqueue_create() in
drivers/gpu/drm/msm/msm_submitqueue.c:

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to ignore the
return value of drm_sched_entity_init()?

In msm_submitqueue_create(), the return value of drm_sched_entity_init() is
ignored:

drm_sched_entity_init(queue->entity, DRM_SCHED_PRIORITY_KERNEL,
                      &sched, 1, NULL);

If initialization fails, the function continues. The subsequent error check
if (IS_ERR(queue->entity)) fails to detect the error because queue->entity
points to an embedded array element, not an ERR_PTR. 

This could lead to using an uninitialized scheduler entity, potentially
causing memory corruption or crashes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729155609.20190-1-robin.clark@oss.qualcomm.com?part=3