Re: [PATCH v3 3/4] drm/bridge: it6505: Add audio support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] TOCTOU race and unsynchronized hardware access in audio FIFO error IRQ handler.
- [High] Use-After-Free of audio codec device during driver removal.
- [High] Uninitialized audio mute state causes premature audio enablement.
--

--- Patch [3]: [PATCH v3 3/4] drm/bridge: it6505: Add audio support ---
commit 6cb75640ad011b0aa639c8886372817492ac07db
Author: Jiaxin Yu <jiaxin.yu@mediatek.com>
Subject: drm/bridge: it6505: Add audio support

This commit adds audio support for the it6505 by bridging to the hdmi-codec.
It registers an hdmi-audio-codec platform device from probe and wires up
audio setup/shutdown helpers via hdmi_codec_ops.

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 f2ed76a05f6bc..37036ef5403fb 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -2627,6 +2635,9 @@ static void it6505_irq_audio_fifo_error(struct it6505 *it6505)
>  
>  	DRM_DEV_DEBUG_DRIVER(dev, "audio fifo error Interrupt");
>  
> +	if (it6505->audio.mute)
> +		return;
> +
> +	if (it6505_audio_input(it6505))
> +		it6505_enable_audio(it6505);
>  }

[Severity: High]
Does this create a race condition where the mute state can be overridden?

Since it6505_irq_audio_fifo_error() checks it6505->audio.mute locklessly,
and it6505_audio_input() sleeps for 3-4ms:

drivers/gpu/drm/bridge/ite-it6505.c:it6505_audio_input() {
    ...
    reg05 = it6505_read(it6505, REG_RESET_CTRL);
    it6505_set_bits(it6505, REG_RESET_CTRL, AUDIO_RESET, 0x00);
    usleep_range(3000, 4000);
    regbe = it6505_read(it6505, REG_AUDIO_INPUT_FREQ);
    it6505_write(it6505, REG_RESET_CTRL, reg05);
    ...
}

If userspace triggers mute_stream(true) during this sleep, it acquires
the audio_lock and disables audio via REG_RESET_CTRL. When
it6505_audio_input() wakes up, it writes back the stale reg05 value.

Then it6505_enable_audio() is called, which grabs the lock and re-enables
audio without re-checking the mute flag. Should the mute flag be checked
again under the lock, or the hardware access synchronized differently?

[ ... ]

> @@ -3056,6 +3104,39 @@ static int it6505_audio_hook_plugged_cb(struct device *dev,
>  					void *data,
>  					hdmi_codec_plugged_cb fn,
>  					struct device *codec_dev)
>  {
>  	struct it6505 *it6505 = data;
>  
>  	it6505->plugged_cb = fn;
>  	it6505->codec_dev = codec_dev;
>  	it6505_plugged_status_to_codec(it6505);
>  
>  	return 0;
>  }
>  
> +static const struct hdmi_codec_ops it6505_audio_codec_ops = {
> +	.hw_params = it6505_audio_hw_params,
> +	.mute_stream = it6505_audio_mute,
> +	.audio_shutdown = it6505_audio_shutdown,
> +	.hook_plugged_cb = it6505_audio_hook_plugged_cb,
> +};
> +
> +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;
> +
> +	INIT_DELAYED_WORK(&it6505->delayed_audio, it6505_delayed_audio);

[Severity: High]
Does this leave it6505->audio.mute initialized to false?

Since the structure is zero-initialized, mute defaults to false before
ALSA configures it. If a FIFO error interrupt fires early, could this
cause it6505_irq_audio_fifo_error() to prematurely enable audio before
valid parameters are set? Should the initial mute state be set to true?

[ ... ]

> @@ -3647,6 +3735,8 @@ static void it6505_i2c_remove(struct i2c_client *client)
>  {
>  	struct it6505 *it6505 = i2c_get_clientdata(client);
>  
> +	platform_device_unregister(it6505->audio_pdev);
> +	cancel_delayed_work_sync(&it6505->delayed_audio);
>  	cancel_work_sync(&it6505->link_works);
>  	cancel_work_sync(&it6505->hdcp_wait_ksv_list);
>  	cancel_delayed_work_sync(&it6505->hdcp_work);

[Severity: High]
Can this cause a use-after-free of the codec device?

Unregistering the platform device frees the codec device, but the threaded
IRQ (which is devm-managed) and extcon_wq are not shut down until later.

If an HPD interrupt fires or the workqueue is already running,
it6505_extcon_work() can call it6505_plugged_status_to_codec():

drivers/gpu/drm/bridge/ite-it6505.c:it6505_plugged_status_to_codec() {
    ...
    if (it6505->plugged_cb && it6505->codec_dev)
        it6505->plugged_cb(it6505->codec_dev,
                           status == connector_status_connected);
    ...
}

This will dereference the freed it6505->codec_dev. Could the platform
device unregistration be moved after the workqueues and IRQ are stopped,
or should the pointers be explicitly cleared?

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