Re: [PATCH 1/2] drm/bridge: it6505: Add audio support

[sashiko-bot@kernel.org]

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

New issues:
- [High] Resource leak and Use-After-Free of hdmi-codec platform device
- [High] Missing synchronous cancellation of `delayed_audio` on driver removal causes Use-After-Free
- [High] Pending `delayed_audio` work is not cancelled during ALSA audio shutdown
- [High] `delayed_audio` workqueue is initialized after device registration
- [High] Concurrent I2C operations in audio enable/disable lack synchronization
- [Medium] Audio workaround logic fails to disable audio when muted

Pre-existing issues:
- [High] Pre-existing missing synchronous cancellation of HDCP work on driver removal
--

--- Patch [1]: [PATCH 1/2] drm/bridge: it6505: Add audio support ---
commit 645bfa307d3295ef683ceb3cf17d99b78c9a13a0
Author: Jiaxin Yu <jiaxin.yu@mediatek.com>

drm/bridge: it6505: Add audio support

Add audio support for it6505 by bridging to the hdmi-codec, registering
an "hdmi-audio-codec" platform device from probe. The it6505's audio
setup/shutdown helpers were merged earlier as unused code; wire them up
via hdmi_codec_ops so the DAI actually appears.

Link: https://lore.kernel.org/all/20230730180803.22570-4-jiaxin.yu@mediatek.com/

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 8ecb43611dba8..698ae01b3a474 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
[ ... ]
> @@ -2960,7 +2953,7 @@ static void it6505_remove_notifier_module(struct it6505 *it6505)
>  	}
>  }
>  
> -static void __maybe_unused it6505_delayed_audio(struct work_struct *work)
> +static void it6505_delayed_audio(struct work_struct *work)
>  {
>  	struct it6505 *it6505 = container_of(work, struct it6505,
>  					     delayed_audio.work);
> @@ -2974,9 +2967,9 @@ static void __maybe_unused it6505_delayed_audio(struct work_struct *work)
>  		it6505_enable_audio(it6505);
>  }

[Severity: High]
With the wiring up of delayed_audio and the hdmi_codec_ops,
it6505_enable_audio() and it6505_disable_audio() can now execute concurrently
from the system workqueue, ALSA process context, and threaded IRQs.

Since both functions perform multi-step unprotected hardware configuration,
could their I2C operations interleave and corrupt the audio configuration
registers? Is a mutex needed to protect these sequences?

[ ... ]
> -static void __maybe_unused it6505_audio_shutdown(struct device *dev, void *data)
> +static void it6505_audio_shutdown(struct device *dev, void *data)
>  {
>  	struct it6505 *it6505 = dev_get_drvdata(dev);
>  
> @@ -3039,10 +3032,40 @@ static void __maybe_unused it6505_audio_shutdown(struct device *dev, void *data)
>  		it6505_disable_audio(it6505);
>  }

[Severity: High]
Does this leave pending delayed work active?

If a user starts the audio stream and closes it within 180ms, the delayed work
might still execute after this shutdown completes, which would re-enable the
audio hardware on a closed stream.

Should this call cancel_delayed_work_sync(&it6505->delayed_audio) before
disabling the audio?

> -static int __maybe_unused it6505_audio_hook_plugged_cb(struct device *dev,
> -						       void *data,
> -						       hdmi_codec_plugged_cb fn,
> -						       struct device *codec_dev)
> +static int it6505_audio_hw_params(struct device *dev, void *data,
> +				  struct hdmi_codec_daifmt *daifmt,
> +				  struct hdmi_codec_params *params)
> +{
> +	struct it6505 *it6505 = dev_get_drvdata(dev);
> +
> +	return it6505_audio_setup_hw_params(it6505, params);
> +}
> +
> +static int it6505_audio_mute(struct device *dev, void *data,
> +			     bool enable, int direction)
> +{
> +	struct it6505 *it6505 = dev_get_drvdata(dev);
> +
> +	DRM_DEV_DEBUG_DRIVER(dev, "mute: %d", enable);
> +
> +	/*
> +	 * Some DP-to-HDMI dongles get into a bad state if the InfoFrame is
> +	 * sent without audio data, so only enable it6505's audio once the
> +	 * stream is unmuted (i.e. actually playing).
> +	 */
> +	if (enable)
> +		cancel_delayed_work(&it6505->delayed_audio);

[Severity: Medium]
Will this fail to disable audio when the stream is muted after being unmuted
for more than 180ms?

If enable is true (which typically means muting in ALSA semantics), and the
delayed work has already executed, canceling the work will do nothing.

Should this actively call it6505_disable_audio() if the stream is being muted?

Also, could using the non-synchronous cancel_delayed_work() introduce a race
condition if the worker is concurrently executing?

> +	else
> +		queue_delayed_work(system_wq, &it6505->delayed_audio,
> +				   msecs_to_jiffies(180));
> +
> +	return 0;
> +}
[ ... ]
> +static int it6505_register_audio_driver(struct device *dev)
> +{
> +	struct it6505 *it6505 = dev_get_drvdata(dev);
> +	struct hdmi_codec_pdata codec_data = {
> +		.ops = &it6505_audio_codec_ops,
> +		.max_i2s_channels = 8,
> +		.i2s = 1,
> +		.no_capture_mute = 1,
> +		.data = it6505,
> +	};
> +	struct platform_device *pdev;
> +
> +	pdev = platform_device_register_data(dev, HDMI_CODEC_DRV_NAME,
> +					     PLATFORM_DEVID_AUTO, &codec_data,
> +					     sizeof(codec_data));
> +	if (IS_ERR(pdev))
> +		return PTR_ERR(pdev);
> +
> +	INIT_DELAYED_WORK(&it6505->delayed_audio, it6505_delayed_audio);

[Severity: High]
Could this result in an uninitialized work structure being used?

If the ALSA driver binds and invokes the mute_stream callback immediately after
platform_device_register_data() returns, queue_delayed_work() could be called
before INIT_DELAYED_WORK() is executed.

[Severity: High]
Does this code leak the platform device on driver unbind?

The returned pdev is discarded, meaning platform_device_unregister() is never
called during driver teardown.

Since the platform device's .data pointer references the struct it6505 allocated
by the I2C driver, could this also lead to a use-after-free when the ALSA
subsystem interacts with the codec after the I2C driver is unbound?

[Severity: High]
When the I2C driver is removed, the newly added delayed_audio work does not
appear to be cancelled anywhere.

Since struct it6505 is freed by devres on unbind, could a pending delayed work
execute on the freed memory and cause a use-after-free?

[Severity: High]
This is a pre-existing issue, but similar to the delayed_audio work, the I2C
driver's remove path appears to be missing synchronous cancellation for
hdcp_work and hdcp_wait_ksv_list.

Could this also lead to a use-after-free if the module is removed while HDCP
authentication is ongoing?

> +	DRM_DEV_DEBUG_DRIVER(dev, "bound to %s", HDMI_CODEC_DRV_NAME);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784393979.git.daniel@makrotopia.org?part=1