Re: [PATCH] drm/omap: Report HDMI hotplug events to the ASoC HDMI codec

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The HPD notification path lacks lifecycle synchronization with the audio driver, resulting in Use-After-Free vulnerabilities during driver unbind. During removal, `hdmi4_unbind()` unregisters and frees `hdmi->audio_pdev`, but the DRM bridge remains registered until `hdmi4_uninit_output()` is called later in `hdmi4_remove()`. An HPD event firing in this window will unconditionally dereference the freed `audio_pdev` in `hdmi4_bridge_hpd_notify()`. Additionally, while `omap_hdmi_audio_remove()` clears `ha->audio_hpd`, it does not synchronize with ongoing executions of `hdmi_audio_hpd()`. Consequently, a concurrent HPD event that passes the NULL check can dereference `struct hdmi_audio_data *ad` after it has been freed by `devres`, leading to a kernel crash.
- [High] The newly added ALSA jack and its kcontrols are never exposed to userspace because the jack is created after the sound card has been registered. In `omap_hdmi_audio_probe()`, `snd_soc_card_jack_new()` is incorrectly called after `devm_snd_soc_register_card()`. `devm_snd_soc_register_card()` internally calls ALSA's `snd_card_register()`, which iterates and registers all attached devices to userspace. ALSA devices created after `snd_card_register()` require an explicit call to `snd_device_register()` to become visible. Because ASoC does not do this for late-added jacks, the jack's input device and kcontrols are never created, completely breaking the intended functionality.
- [Medium] The patch intends to report HDMI hotplug events to the `omap-hdmi-audio` ASoC codec, which is shared by both OMAP4 and OMAP5/DRA7 platforms. However, the HPD notification callback (`.hpd_notify`) was only implemented for the HDMI4 bridge in `hdmi4.c`. The `hdmi5_bridge_funcs` structure in `hdmi5.c` was completely omitted. Consequently, the hotplug detection feature will remain entirely non-functional on all OMAP5 and DRA7 hardware.
--

--- Patch [1]: [PATCH] drm/omap: Report HDMI hotplug events to the ASoC HDMI codec ---
commit afacde67dc4a81e706a39b2252a2c9a3309fb284
Author: Ivaylo Dimitrov <[email protected]>

drm/omap: Report HDMI hotplug events to the ASoC HDMI codec

This patch adds an optional HPD callback to omap_hdmi_audio_pdata and
invokes it from the DRM HDMI bridge whenever the connector status changes.
The HDMI audio driver registers an ASoC jack to report these state changes,
and the disconnect path is fixed to take a runtime PM reference.

> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4.c b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
> index e306247ed8a05..b466bbdae5ccc 100644
> --- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c
> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
> @@ -432,9 +432,19 @@ static void hdmi4_bridge_hpd_notify(struct drm_bridge *bridge,
>  				    enum drm_connector_status status)
>  {
>  	struct omap_hdmi *hdmi = drm_bridge_to_hdmi(bridge);
> +	struct device *dev = &hdmi->audio_pdev->dev;
> +	struct omap_hdmi_audio_pdata *ha = dev_get_platdata(dev);

[Severity: High]
Does this introduce a use-after-free risk during driver unbind?

Since hdmi4_unbind() unregisters and frees hdmi->audio_pdev, but the DRM
bridge remains registered until hdmi4_remove() is called later, could an
HPD event firing in this window unconditionally dereference the freed
audio_pdev?

[Severity: Medium]
Since this adds the HPD notification callback for the HDMI4 bridge, should
the hdmi5_bridge_funcs in hdmi5.c also implement the .hpd_notify callback so
that the hotplug detection feature functions on OMAP5 and DRA7 hardware?

> +
> +	if (ha->audio_hpd)
> +		ha->audio_hpd(dev, status == connector_status_connected);
> +
> +	if (status == connector_status_disconnected) {
> +		if (hdmi_runtime_get(hdmi))
> +			return;
>  
> -	if (status == connector_status_disconnected)
>  		hdmi4_cec_set_phys_addr(&hdmi->core, CEC_PHYS_ADDR_INVALID);
> +		hdmi_runtime_put(hdmi);
> +	}
>  }

[ ... ]

> diff --git a/sound/soc/ti/omap-hdmi.c b/sound/soc/ti/omap-hdmi.c
> index e60f5b483fc57..333aa308979e5 100644
> --- a/sound/soc/ti/omap-hdmi.c
> +++ b/sound/soc/ti/omap-hdmi.c

[ ... ]

> @@ -380,19 +395,37 @@ static int omap_hdmi_audio_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	atomic_set(&ad->jack_state, -1);
>  	ad->card = card;
>  	snd_soc_card_set_drvdata(card, ad);
>  
>  	dev_set_drvdata(dev, ad);
>  
> +	ret = snd_soc_card_jack_new(
> +		      card, "HDMI", SND_JACK_AVOUT, &ad->jack);
> +	if (ret < 0) {

[Severity: High]
Is the jack actually exposed to userspace when created here?

In omap_hdmi_audio_probe(), devm_snd_soc_register_card() has already been
called earlier in the function. Since the card is already registered,
wouldn't calling snd_soc_card_jack_new() after registration cause the jack
to be excluded from the device registration process, meaning the input
device and kcontrols are never created?

> +		dev_err(dev, "Cannot create HDMI jack: %i\n", ret);
> +		return ret;
> +	}
> +
> +	ha->audio_hpd = hdmi_audio_hpd;
> +
>  	return 0;
>  }
>  
> +static void omap_hdmi_audio_remove(struct platform_device *pdev)
> +{
> +	struct omap_hdmi_audio_pdata *ha = pdev->dev.platform_data;
> +
> +	ha->audio_hpd = NULL;
> +}

[Severity: High]
Could this lead to a use-after-free if a concurrent HPD event is executing?

While this clears ha->audio_hpd, it doesn't appear to synchronize with
ongoing executions of hdmi_audio_hpd(). Could a concurrent HPD event pass
the NULL check and dereference the ad struct after devres has freed it?

> +
>  static struct platform_driver hdmi_audio_driver = {
>  	.driver = {
>  		.name = DRV_NAME,

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