Re: [PATCH v2 2/2] drm/bridge: it6505: Don't reject audio hw_params without an encoder

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing bounds check for `channel_count == 0` causes an out-of-bounds stack read in `it6505_enable_audio_infoframe`.
- [High] Missing cancellation of workqueues in the driver's remove path leads to Use-After-Free.
--

--- Patch [2]: [PATCH v2 2/2] drm/bridge: it6505: Don't reject audio hw_params without an encoder ---
commit 283dcf1804abd2952ffc7edbd9e7c45ae0570f87
Author: Daniel Golle <daniel@makrotopia.org>

drm/bridge: it6505: Don't reject audio hw_params without an encoder

This commit drops the encoder check in it6505_audio_setup_hw_params() so
stream parameters are always cached. Previously, returning early left
it6505->audio.channel_count at 0, which could cause an out-of-bounds access
if a display was hotplugged while a stream was running.

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index d84441926e8e0..e20edbbb7c133 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -2979,9 +2979,6 @@ static int it6505_audio_setup_hw_params(struct it6505 *it6505,
>  			     params->sample_rate, params->sample_width,
>  			     params->cea.channels);
>  
> -	if (!it6505->bridge.encoder)
> -		return -ENODEV;
> -

[Severity: High]
This is a pre-existing issue, but if the SoC's I2S controller continuously
drives clocks (e.g. from an active shared speaker backend), could the IT6505
hardware audio FIFO overflow and trigger the INT_AUDIO_FIFO_ERROR interrupt
before ALSA configures the audio stream parameters?

If this happens, it6505_irq_audio_fifo_error() would call
it6505_enable_audio_infoframe() while it6505->audio.channel_count is still 0
(zero-initialized during probe). 

In it6505_enable_audio_infoframe(), it6505->audio.channel_count - 1 evaluates
to -1, causing an out-of-bounds stack read:

    audio_info_ca[it6505->audio.channel_count - 1]);

>  	if (params->cea.channels <= 1 || params->cea.channels > 8) {
>  		DRM_DEV_DEBUG_DRIVER(dev, "channel number: %d not support",
>  				     it6505->audio.channel_count);

[Severity: High]
This is a pre-existing issue, but does the driver's remove path miss cancelling
several workqueues, leading to a potential use-after-free?

In it6505_i2c_remove():

    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);
        drm_bridge_remove(&it6505->bridge);
        drm_dp_aux_unregister(&it6505->aux);
        it6505_debugfs_remove(it6505);
        it6505_poweroff(it6505);
        it6505_remove_edid(it6505);
    }

Only delayed_audio is explicitly cancelled. 

If other background work items (link_works, hdcp_wait_ksv_list, hdcp_work,
extcon_wq) are pending or running when the device is unbound, could they access
the it6505 structure after it has been automatically freed by devres?

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