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 <[email protected]>
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/[email protected]?part=11
