Re: [PATCH] drm/vc4: Fix firmware reference leak in vc4_hvs_bind()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] drm/vc4: resource leak and NULL pointer dereference in
  vc4_hvs_bind() [1]
--

commit 17330184776a3fb8ffab5b0b09aa15bba8f8bd33
Author: Wentao Liang <vulab@iscas.ac.cn>

drm/vc4: Fix firmware reference leak in vc4_hvs_bind()

This commit moves the rpi_firmware_find_node() and rpi_firmware_get()
calls in vc4_hvs_bind() below the devm_clk_get() lookups so the firmware
reference is not leaked when clock acquisition fails.

> diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
> index b1fb096858ccc..537aaa9921293 100644
> --- a/drivers/gpu/drm/vc4/vc4_hvs.c
> +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
> @@ -1677,15 +1677,6 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
>  		struct device_node *node;
>  		unsigned int max_rate;
>  
> -		node = rpi_firmware_find_node();
> -		if (!node)
> -			return -EINVAL;
> -
> -		firmware = rpi_firmware_get(node);
> -		of_node_put(node);
> -		if (!firmware)
> -			return -EPROBE_DEFER;
> -
>  		hvs->core_clk = devm_clk_get(&pdev->dev,
>  					     (vc4->gen >= VC4_GEN_6_C) ? "core" : NULL);
>  		if (IS_ERR(hvs->core_clk)) {
> @@ -1700,6 +1691,15 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
>  			return PTR_ERR(hvs->disp_clk);
>  		}
>  
> +		node = rpi_firmware_find_node();
> +		if (!node)
> +			return -EINVAL;
> +
> +		firmware = rpi_firmware_get(node);
> +		of_node_put(node);
> +		if (!firmware)
> +			return -EPROBE_DEFER;
> +
>  		max_rate = rpi_firmware_clk_get_max_rate(firmware,
>  							 RPI_FIRMWARE_CORE_CLK_ID);
>  		rpi_firmware_put(firmware);

[Severity: High]
This problem wasn't introduced by this patch, but can vc4_hvs_irq_handler()
dereference a NULL vc4->hvs pointer after component unbind [1]?

vc4_hvs_bind() allocates hvs and registers vc4_hvs_irq_handler() with
devm_kzalloc() and devm_request_irq() tied to the component platform
device rather than the aggregate DRM lifetime.  When the component
unbinds (for example on aggregate probe deferral), vc4_hvs_unbind() sets
vc4->hvs to NULL while the devres-managed IRQ handler remains registered:

drivers/gpu/drm/vc4/vc4_hvs.c:vc4_hvs_irq_handler() {
	...
	struct vc4_dev *vc4 = to_vc4_dev(dev);
	...
	status = HVS_READ(SCALER_DISPSTAT);
	...
}

If an interrupt fires after unbind, HVS_READ() dereferences
vc4->hvs->regs, causing a NULL pointer dereference, and a subsequent
re-bind leaks devres allocations and fails in devm_request_irq() with
-EBUSY.

[1] https://sashiko.dev/#/bug/linux-4ad12be9-d1a5-441d-9845-195d42c3f880

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106092016.2282805-1-vulab@iscas.ac.cn?part=1