Re: [PATCH] drm/vc4: hdmi: Unregister the ASoC card on unbind

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

Pre-existing issues:
- [High] Incomplete Fix / Use-After-Free / Resource Leak: The patch fixes a Use-After-Free for the ASoC card by replacing `devm_snd_soc_register_card` with a non-devres registration managed in `.unbind`. However, it leaves numerous other `devm_` allocations in `vc4_hdmi_bind()`, `vc4_hdmi_audio_init()`, `vc4_hdmi_hotplug_init()`, and `vc4_hdmi_cec_register()` tied to the component's platform device (`dev`) while passing pointers to the `drmm_kzalloc`-allocated `vc4_hdmi` structure. When the DRM aggregate device is unbound (e.g., via `rmmod vc4`), the master device's release runs and frees `vc4_hdmi`. The platform device, whose lifecycle is separate, retains the `devm_` actions (like `vc4_hdmi_put_ddc` and IRQ handlers) in its devres queue. When the platform device is eventually removed, or if an active interrupt fires after the DRM device unbinds, these actions access the freed `vc4_hdmi` pointer, causing a Use-After-Free. Additionally, unbinding and rebinding the component will stack duplicate devres actions on the platform device, causing memory leaks.
--

--- Patch [1]: [PATCH] drm/vc4: hdmi: Unregister the ASoC card on unbind ---
commit 197ce798da149a07ddab65100d64653586ed2892
Author: Karl Mehltretter <[email protected]>

drm/vc4: hdmi: Unregister the ASoC card on unbind

The patch fixes a use-after-free issue by switching the ASoC card
registration from devm to manual registration and unregistering it during
the component unbind callback.

> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 17c8635c5afae..ce28075f77b68 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -2349,28 +2349,11 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)

[ ... ]

> +	 * The card is unregistered from the component unbind callback:
> +	 * a DRM-managed action can run after the device-managed ASoC
> +	 * resources are gone, and the device-managed release runs after
> +	 * the DRM-managed vc4_hdmi structure holding the card has been
> +	 * freed. Only at unbind time are both still alive.
>  	 */
>  	ret = devm_snd_dmaengine_pcm_register(dev, &pcm_conf, 0);

[Severity: High]
This isn't a bug introduced by this patch, but while this commit fixes the UAF
for the ASoC card, does leaving devm_snd_dmaengine_pcm_register() and
devm_snd_soc_register_component() tied to the platform device (dev) leave
them vulnerable to a similar issue?

When the DRM master device is unbound, the drmm-managed vc4_hdmi structure
is freed. However, the HDMI platform device has a separate lifecycle, so these
devm_ resources won't be released until the platform device is removed. 

If the component is unbound and rebound, wouldn't these devm_ registrations
stack and leak memory?

[ ... ]

> @@ -3345,8 +3329,20 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
>  	return ret;
>  }
>  
> +static void vc4_hdmi_unbind(struct device *dev, struct device *master,
> +			    void *data)
> +{
> +	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
> +
> +	if (vc4_hdmi->audio.card_registered) {
> +		snd_soc_unregister_card(&vc4_hdmi->audio.card);
> +		vc4_hdmi->audio.card_registered = false;
> +	}
> +}

[Severity: High]
This is a pre-existing issue, but since the vc4_hdmi structure is passed
to several devm_ actions in the bind path (such as vc4_hdmi_put_ddc, and
IRQ handlers for HPD and CEC), wouldn't those callbacks dereference the
freed vc4_hdmi pointer if the platform device is removed after the DRM
master device has already been torn down?

For example, vc4_hdmi_hotplug_init() registers the HPD irq handler using
devm_request_threaded_irq() against the platform device. If an interrupt
fires after the DRM device is unbound, won't it try to access the freed
vc4_hdmi pointer?

Should these other resources also be manually cleaned up in the .unbind
callback, or transitioned to drmm_ management?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1