Re: [PATCH v4 4/5] drm/bridge: it6505: Add audio support

[sashiko-bot@kernel.org]

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 <jiaxin.yu@mediatek.com>

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/20230730180803.22570-4-jiaxin.yu@mediatek.com/

> 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/cover.1784681487.git.daniel@makrotopia.org?part=4