Re: [PATCH] drm/vc4: hvs/v3d: Fix null dereference in unbind

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] vc4_v3d_unbind() accesses hardware without resuming from runtime suspend, leading to system hangs.
- [High] Component bind callbacks improperly use devm_ APIs on the component device, causing memory leaks and dangling IRQ handlers.
--

--- Patch [1]: [PATCH] drm/vc4: hvs/v3d: Fix null dereference in unbind ---
commit 70be44b543f0237e5db3dfd6dff591c323b131ef
Author: Gregor Herburger <gregor.herburger@linutronix.de>

drm/vc4: hvs/v3d: Fix null dereference in unbind

The hvs and v3d drivers use dev_get_drvdata(master) in their unbind
functions. Since the vc4-drm gets removed before its dependent drivers
(vc4_hvs/vc4_v3d) the vc4_hvs_unbind/vc4_v3d_unbind functions try to
get drvdata of its master and fails with a null dereference error.

> diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
> index 184d51ea3fa57..e715147d091ca 100644
> --- a/drivers/gpu/drm/vc4/vc4_hvs.c
> +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
> @@ -1752,7 +1752,7 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
>  static void vc4_hvs_unbind(struct device *dev, struct device *master,
>  			   void *data)
>  {
> -	struct drm_device *drm = dev_get_drvdata(master);
> +	struct drm_device *drm = data;

[Severity: High]
This is a pre-existing issue, but does vc4_hvs_bind() improperly use
devm_ APIs?

The component framework leaves the component device bound to its base driver
even when the aggregate DRM master unbinds. Since devm_ APIs are attached
to the component device (dev), they are only freed when the component device
is removed, not when it is unbound.

Specifically, devm_request_irq() in vc4_hvs_bind() leaves the IRQ handler
registered after unbind. Since vc4_hvs_unbind() nullifies vc4->hvs, will
a subsequent interrupt cause vc4_hvs_irq_handler() to dereference a NULL
pointer?

>  	struct vc4_dev *vc4 = to_vc4_dev(drm);
>  	struct vc4_hvs *hvs = vc4->hvs;
>  	struct drm_mm_node *node, *next;
> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
> index d31b906cb8e78..f32410420d3e4 100644
> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
> @@ -494,7 +494,7 @@ static int vc4_v3d_bind(struct device *dev, struct device *master, void *data)
>  static void vc4_v3d_unbind(struct device *dev, struct device *master,
>  			   void *data)
>  {
> -	struct drm_device *drm = dev_get_drvdata(master);
> +	struct drm_device *drm = data;

[Severity: High]
This is a pre-existing issue, but does vc4_v3d_bind() leak memory on unbind?

Since devm_ APIs attached to the component device (&pdev->dev) are only freed
when the component device is removed (not unbound), does the devm_kzalloc()
in vc4_v3d_bind() leak memory on every unbind/bind cycle?

>  	struct vc4_dev *vc4 = to_vc4_dev(drm);
>  
>  	vc4_irq_uninstall(drm);

[Severity: High]
This is a pre-existing issue, but can this code access hardware while it is
runtime suspended?

When the DRM driver is unbound, vc4_v3d_unbind() is called. The V3D hardware
may be in an autosuspended state because vc4_v3d_bind() enables runtime PM
autosuspend.

This calls vc4_irq_uninstall() (which accesses V3D_INTDIS and V3D_INTCTL)
and subsequently writes to V3D_BPOA and V3D_BPOS.

Will accessing these registers without a preceding pm_runtime_get_sync() to
wake the hardware cause an AXI bus timeout and system hang on ARM systems?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-rpi-vc4-fix-v1-1-6073a96a5773@linutronix.de?part=1