Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Unconditional `pm_runtime_resume_and_get()` on `gmu->gxpd` causes a NULL pointer dereference if the gxpd power domain is absent. - [High] The `opp` pointer is used after its reference count has been dropped, creating a Use-After-Free vulnerability. -- commit 8f5f79a7915a90cb43eb05a5eb535e070ae669bd Author: Roman Demidov <[email protected]> drm/msm/a6xx: check pm_runtime_resume_and_get() during resume This commit adds return value checks to pm_runtime_resume_and_get() calls during a6xx GPU resume to prevent hardware access on suspended devices. It also unwinds acquired resources properly if these calls fail. > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > index f9de9329dee39..0cf205ea744be 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > @@ -2175,44 +2175,48 @@ static int a6xx_pm_resume(struct msm_gpu *gpu) > opp = dev_pm_opp_find_freq_ceil(&gpu->pdev->dev, &freq); > if (IS_ERR(opp)) { > ret = PTR_ERR(opp); > - goto err_set_opp; > + goto err_unlock; > } > dev_pm_opp_put(opp); > > /* Set the core clock and bus bw, having VDD scaling in mind */ > dev_pm_opp_set_opp(&gpu->pdev->dev, opp); [Severity: High] This is a pre-existing issue, but does this code cause a use-after-free of the opp pointer? The dev_pm_opp_find_freq_ceil() function returns an OPP object with an incremented reference count. Dropping this reference via dev_pm_opp_put() could cause the object to be freed if it is concurrently removed from the OPP table. Can dev_pm_opp_set_opp() then dereference the freed memory? > > - pm_runtime_resume_and_get(gmu->dev); > - pm_runtime_resume_and_get(gmu->gxpd); > + ret = pm_runtime_resume_and_get(gmu->dev); > + if (ret < 0) > + goto err_opp_clear; > + ret = pm_runtime_resume_and_get(gmu->gxpd); [Severity: High] This isn't a bug introduced by this patch, but can this cause a NULL pointer dereference if the gxpd power domain is absent? If the driver initializes a GMU wrapper device on a platform where the device tree does not provide a 'gx' power domain, gmu->gxpd remains NULL. Calling pm_runtime_resume_and_get(NULL) will immediately dereference the pointer inside __pm_runtime_resume(). > + if (ret < 0) > + goto err_put_dev; [ ... ] > - /* If anything goes south, tear the GPU down piece by piece.. */ > - if (ret) { > -err_bulk_clk: > - pm_runtime_put(gmu->gxpd); > - pm_runtime_put(gmu->dev); > - dev_pm_opp_set_opp(&gpu->pdev->dev, NULL); > - } > -err_set_opp: > mutex_unlock(&a6xx_gpu->gmu.lock); > + msm_devfreq_resume(gpu); > + a6xx_llc_activate(a6xx_gpu); > > - if (!ret) { > - msm_devfreq_resume(gpu); > - a6xx_llc_activate(a6xx_gpu); > - } > + return 0; > > + /* If anything goes south, tear the GPU down piece by piece.. */ > +err_put_gxpd: > + pm_runtime_put(gmu->gxpd); [Severity: High] Can this error path also crash by unconditionally dereferencing a NULL gxpd pointer? While the initial pm_runtime_resume_and_get() issue is pre-existing, this patch introduces an additional crash point here. If gmu->gxpd is NULL, this newly added pm_runtime_put() will trigger a kernel panic. > +err_put_dev: > + pm_runtime_put(gmu->dev); > +err_opp_clear: > + dev_pm_opp_set_opp(&gpu->pdev->dev, NULL); > +err_unlock: > + mutex_unlock(&a6xx_gpu->gmu.lock); > return ret; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
