Re: [PATCH v3 4/4] 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 5 potential issue(s) to consider:

Pre-existing issues:
- [High] The interrupt handler can still execute and re-queue work items after `cancel_work_sync()` in `it6505_i2c_remove()`, leading to Use-After-Free.
- [High] Missing `pm_runtime_disable()` in `it6505_i2c_remove()` leaves the device in an inconsistent PM state.
- [High] Unprotected modification and TOCTOU race on `it6505->audio.mute` leading to unintended audio enablement.
- [Medium] Cached audio parameters are overwritten by default values on display hotplug, defeating the secondary goal of the patch.
- [Medium] Audio configuration fields in `it6505->audio` are modified without consistent locking, leading to potential race conditions.
--

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

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

it6505_audio_setup_hw_params() returns -ENODEV when the bridge is not
attached to a DRM encoder. This commit drops the encoder check so the
parameters are always cached.

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 37036ef5403fb..435601b6929ed 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -2999,9 +2999,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: Medium]
This is a pre-existing issue, but does removing this check fully solve the
problem if the cached parameters are overwritten on hotplug?

When a stream starts with no display attached, it6505_audio_setup_hw_params()
caches the correct settings as intended. However, if a display is hotplugged
later, the HPD interrupt handler sees a disconnected state beforehand and zeroed
it6505->dpcd.

Upon reconnect, it finds it6505->dpcd[0] == 0 and calls
it6505_variable_config():

    it6505->audio.sample_rate = AUDIO_SAMPLE_RATE;
    it6505->audio.channel_count = AUDIO_CHANNEL_COUNT;

Does this unconditionally reset the audio variables to default values, causing
audio to play with 2 channels and 48kHz rather than the dynamically configured
stream parameters?

[Severity: Medium]
This is a pre-existing issue, but does this function need to take the
audio_lock when modifying the audio configuration fields?

In it6505_audio_setup_hw_params():

    it6505->audio.channel_count = params->cea.channels;

This modifies the fields in it6505->audio without taking it6505->audio_lock.

Since it6505_enable_audio() can be called from asynchronous paths (like
it6505_delayed_audio or IRQ) which takes audio_lock and reads these fields,
could this result in hardware being programmed with a partially updated state
if readers see a mix of old and new parameters?

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

[Severity: High]
This is a pre-existing issue, but can the interrupt handler still execute and
re-queue work items after they are cancelled during driver unbind?

In it6505_i2c_remove():

    cancel_work_sync(&it6505->link_works);
    cancel_work_sync(&it6505->hdcp_wait_ksv_list);
    cancel_delayed_work_sync(&it6505->hdcp_work);
    cancel_work_sync(&it6505->extcon_wq);

Since the device interrupt (it6505->irq) is not disabled before cancelling
these work items, could an incoming interrupt spawn the threaded handler and
call schedule_work(&it6505->link_works) after cancel_work_sync() returns,
leading to a use-after-free when the work item executes?

[Severity: High]
This is a pre-existing issue, but is there a missing pm_runtime_disable() in
the driver removal path?

While it6505_i2c_probe() successfully enables runtime PM with
pm_runtime_enable(dev), it6505_i2c_remove() does not appear to call
pm_runtime_disable(dev) before returning. Can this leave the PM core with
references to the unbound device, leading to a resource leak or lifecycle
violation?

[Severity: High]
This is a pre-existing issue, but is there a time-of-check to time-of-use race
on the audio mute state?

In it6505_irq_audio_fifo_error(), the mute state is read outside of the lock:

    if (it6505->audio.mute)
        return;

    if (it6505_audio_input(it6505))
        it6505_enable_audio(it6505);

If ALSA mutes the stream concurrently, it6505_audio_mute() sets mute to true
and disables audio. After it returns, if the IRQ wakes up and unconditionally
calls it6505_enable_audio() which takes the lock and turns audio back on,
could this defeat the mute and cause hardware malfunction by sending an
InfoFrame without audio data?

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