Re: [PATCH v6 11/16] drm/msm/gem: Set resv before exposing obj

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The patch modifies the signature of msm_gem_new() from 3 to 4 arguments but fails to update the caller in drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c. This results in a compiler error ('too few arguments to function').

Pre-existing issues:
- [Critical] Missing NULL check for the return value of msm_context_vm() leads to a kernel panic when dereferenced by drm_gpuvm_resv_obj(). This problem wasn't introduced by this patch, but remains a critical vulnerability.
--

--- Patch [11]: [PATCH v6 11/16] drm/msm/gem: Set resv before exposing obj ---
commit 96533966332980b8f99abb5ba08db109f97b565b
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/gem: Set resv before exposing obj

Don't swap the resv object _after_ exposing the newly created obj in LRU
or global objects list, as that creates a race condition where another
thread could lock the object using the original (per-obj) resv, but then
unlock after the resv is replaced.

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 961e22d3c7292..12d9b8a9c360a 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c

[ ... ]

> @@ -1135,10 +1135,17 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>  		size_t size, uint32_t flags, uint32_t *handle,
>  		char *name)
>  {
> -	struct drm_gem_object *obj;
> +	struct drm_gem_object *obj, *r_obj = NULL;
>  	int ret;
>  
> -	obj = msm_gem_new(dev, size, flags);
> +	if (flags & MSM_BO_NO_SHARE) {
> +		struct msm_context *ctx = file->driver_priv;
> +		struct drm_gpuvm *vm = msm_context_vm(dev, ctx);
> +
> +		r_obj = drm_gpuvm_resv_obj(vm);

[Severity: Critical]
This is a pre-existing issue, but can msm_context_vm() return NULL here?

If this runs on a KMS-only configuration where no GPU is bound, 
msm_context_vm() can return NULL. When passed to drm_gpuvm_resv_obj(vm),
would this cause a NULL pointer dereference since the macro expands to
((vm)->r_obj)?

Since unprivileged userspace can trigger this path via the MSM_GEM_NEW
ioctl with the MSM_BO_NO_SHARE flag, should a NULL check be added before
accessing vm?

> +	}
> +
> +	obj = msm_gem_new(dev, size, flags, r_obj);

[ ... ]

> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> index 762e546d25ef3..47ff2bbc19150 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -296,7 +296,7 @@ int msm_gem_cpu_fini(struct drm_gem_object *obj);
>  int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>  		size_t size, uint32_t flags, uint32_t *handle, char *name);
>  struct drm_gem_object *msm_gem_new(struct drm_device *dev,
> -		size_t size, uint32_t flags);
> +		size_t size, uint32_t flags, struct drm_gem_object *r_obj);

[Severity: Low]
Does this change miss updating some callers? Looking at mdp4_kms_init() in
drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c, it still calls msm_gem_new() with
only 3 arguments:

mdp4_kms->blank_cursor_bo = msm_gem_new(dev, SZ_16K, MSM_BO_WC | MSM_BO_SCANOUT);

Could this cause a build failure when CONFIG_DRM_MSM_MDP4 is enabled due to
too few arguments?

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