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 <[email protected]> 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/[email protected]?part=2
