Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Reordering PM teardown in device removal and probe error paths creates race conditions (UAF/kernel panics) and violates PM domain rules. Pre-existing issues: - [High] Improper DRM device unplug allows subsequent ioctls to access freed driver resources, causing Use-After-Free. -- commit 385821af932c300aa013c14b13508c8e9cf10f64 Author: Adrián Larumbe <[email protected]> drm/panfrost: Fix PM usage_count mishandling This commit addresses a PM usage_count mishandling issue during device probe and unplug. It adds balanced PM get/put calls to prevent devices from staying powered on indefinitely or triggering constant devfreq OPP transition notifications. > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c > b/drivers/gpu/drm/panfrost/panfrost_drv.c > index 2d4b6aa95c66d..545fbf2c8d0cb 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c [ ... ] > @@ -1000,10 +1001,12 @@ static int panfrost_probe(struct platform_device > *pdev) > if (err < 0) > goto err_out1; > > + pm_runtime_put_autosuspend(pfdev->base.dev); > > return 0; > > err_out1: > + pm_runtime_put_noidle(pfdev->base.dev); > pm_runtime_disable(pfdev->base.dev); > panfrost_device_fini(pfdev); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Does dropping the usage count before pm_runtime_disable() create a race condition where the suspend callback can run and disable clocks before hardware shutdown? Because the usage count is dropped early, a concurrent PM event could trigger the suspend callback, disabling clocks. Then, panfrost_device_fini() calls panfrost_gpu_fini() which writes to MMIO registers. Could writing to unclocked registers on ARM SoCs cause fatal bus errors or panics? > pm_runtime_set_suspended(pfdev->base.dev); [ ... ] > @@ -1018,8 +1021,9 @@ static void panfrost_remove(struct platform_device > *pdev) > drm_dev_unregister(&pfdev->base); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] This is a pre-existing issue, but does using drm_dev_unregister() instead of drm_dev_unplug() allow subsequent ioctls to access freed driver resources? Because drm_dev_unregister() removes the device from sysfs but does not set the dev->unplugged flag, the DRM core (drm_ioctl_kernel) does not block ioctls from userspace processes that already hold an open file descriptor. Since panfrost_remove() proceeds to call panfrost_device_fini() which destroys critical structures (job manager, MMU contexts, shrinker lists) and powers down hardware, could a user issuing an ioctl during or after driver unbind via sysfs access freed structures and unmapped MMIO regions, resulting in a use-after-free? > > pm_runtime_get_sync(pfdev->base.dev); > - pm_runtime_disable(pfdev->base.dev); > panfrost_device_fini(pfdev); > + pm_runtime_put_noidle(pfdev->base.dev); > + pm_runtime_disable(pfdev->base.dev); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Does placing pm_runtime_put_noidle() and pm_runtime_disable() after panfrost_device_fini() create a window where runtime PM can access freed structures? pm_runtime_put_noidle() drops the PM usage count to 0. At this point, a concurrent PM event (e.g., userspace writing to /sys/.../power/control) can trigger panfrost_device_runtime_suspend() before pm_runtime_disable() executes. Since panfrost_device_fini() has already destroyed driver structures like the job manager, would the suspend callback access freed memory (like checking panfrost_jm_is_idle), causing a use-after-free? > pm_runtime_set_suspended(pfdev->base.dev); > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
