Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Data race and dangling pointer dereference (Use-After-Free) in `it6505_plugged_status_to_codec`. Pre-existing issues: - [High] Use-After-Free of device_node `ep` in `it6505_parse_dt`. - [High] Potential divide-by-zero kernel panic if the audio input frequency register reads as 0. -- --- Patch [4]: [PATCH v4 4/5] drm/bridge: it6505: Add audio support --- commit cb584e38b3bb2386c2c945750f3e9126c5d8acfa Author: Jiaxin Yu <[email protected]> drm/bridge: it6505: Add audio support This commit adds audio support for the it6505 bridge by registering an hdmi-audio-codec platform device. It wires up the previously unused audio setup and shutdown helpers via hdmi_codec_ops. The patch also introduces locking to serialize audio enable/disable paths across different contexts. 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 > --- a/drivers/gpu/drm/bridge/ite-it6505.c > +++ b/drivers/gpu/drm/bridge/ite-it6505.c [ ... ] > @@ -1629,6 +1640,14 @@ static void it6505_enable_audio(struct it6505 *it6505) > it6505_set_bits(it6505, REG_DATA_MUTE_CTRL, EN_AUD_MUTE, 0x00); > } [Severity: High] This is a pre-existing issue, but since this patch wires up the audio helpers and makes them reachable, can this code cause a divide-by-zero panic? In __it6505_enable_audio(), the variable regbe is read from the hardware: regbe = it6505_read(it6505, REG_AUDIO_INPUT_FREQ); DRM_DEV_DEBUG_DRIVER(dev, "regbe:0x%02x audio input fs: %d.%d kHz", regbe, 6750 / regbe, (6750 % regbe) * 10 / regbe); If the hardware register reads as 0 (e.g., due to an uninitialized clock or bus error), regbe will be 0. If CONFIG_DRM_USE_DYNAMIC_DEBUG is disabled, the macro expands to a standard function call, meaning the division 6750 / regbe is unconditionally evaluated before the call, triggering a kernel panic. [ ... ] > -static int __maybe_unused it6505_audio_hook_plugged_cb(struct device *dev, > - void *data, > - hdmi_codec_plugged_cb fn, > - struct device *codec_dev) > +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; [Severity: High] Can this cause a use-after-free if the hdmi-audio-codec driver is unbound concurrently? When the codec driver unbinds, its remove function calls this hook to set it6505->plugged_cb and it6505->codec_dev to NULL. However, this is done without holding mode_lock. Concurrently, if an HPD interrupt or DRM ioctl invokes it6505_bridge_detect(), it calls it6505_plugged_status_to_codec(): static void it6505_plugged_status_to_codec(struct it6505 *it6505) { enum drm_connector_status status = it6505->connector_status; if (it6505->plugged_cb && it6505->codec_dev) it6505->plugged_cb(it6505->codec_dev, ...); } Could the detect thread pass the if-condition, get preempted while the codec unbinds and its module unloads, and then resume to call the now-dangling function pointer with the freed codec_dev? [ ... ] > @@ -3616,6 +3712,12 @@ static int it6505_i2c_probe(struct i2c_client *client) > return err; > } > > + err = it6505_register_audio_driver(dev); > + if (err < 0) { > + dev_err(dev, "Failed to register audio driver: %d", err); > + return err; > + } > + [Severity: High] This is a pre-existing issue, but does this code lead to a use-after-free during device probe? In it6505_parse_dt() (which is called earlier in it6505_i2c_probe()), the ep node is released via of_node_put(ep) and subsequently dereferenced: ep = of_graph_get_endpoint_by_regs(np, 1, 0); of_node_put(ep); if (ep) { len = it6505_get_data_lanes_count(ep, 1, 4); If of_node_put(ep) drops the last reference, the node is freed, and the subsequent call to it6505_get_data_lanes_count(ep, ...) will access freed memory. > INIT_WORK(&it6505->link_works, it6505_link_training_work); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
