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