Re: [PATCH v4 3/5] drm/bridge: it6505: guard against zero channel count in audio infoframe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Kernel panic due to division by zero when calculating audio input frequency.
- [High] Kernel panic due to division by zero when calculating video pixel clock.
--

--- Patch [3]: [PATCH v4 3/5] drm/bridge: it6505: guard against zero channel count in audio infoframe ---
commit 8ace85a52fcbf9bce4f2a154064b6799263b545d
Author: Daniel Golle <daniel@makrotopia.org>
Subject: drm/bridge: it6505: guard against zero channel count in audio infoframe

This commit guards against a zero channel count in it6505_enable_audio_infoframe()
to prevent an out-of-bounds stack read. It also fixes a debug print logging an
incorrect channel count on failure.

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 2a8298ee6c03c..443ea55797441 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c

[ ... ]

While reviewing this, I noticed a couple of pre-existing issues in this file:

drivers/gpu/drm/bridge/ite-it6505.c:it6505_calc_video_info() {
	...
	if (sum == 0) {
		DRM_DEV_DEBUG_DRIVER(dev, "calc video timing error");
		return;
	}

	sum /= 3;
	pclk = 13500 * 2048 / sum;
	...
}

[Severity: High]
This is a pre-existing issue, but can this integer division shrink small sums
to 0?

If the accumulated readings are very small (e.g., sum is 1 or 2), the
integer division sum /= 3 will truncate the sum to 0.

The subsequent division for pclk will divide by zero and panic the kernel.

Should there be a check to ensure sum is still non-zero after the division
by 3?

[ ... ]

> @@ -1575,6 +1575,9 @@ static void it6505_enable_audio_infoframe(struct it6505 *it6505)
>  	struct device *dev = it6505->dev;
>  	u8 audio_info_ca[] = { 0x00, 0x00, 0x01, 0x03, 0x07, 0x0B, 0x0F, 0x1F };
>  
> +	if (!it6505->audio.channel_count)
> +		return;
> +
>  	DRM_DEV_DEBUG_DRIVER(dev, "infoframe channel_allocation:0x%02x",
>  			     audio_info_ca[it6505->audio.channel_count - 1]);
>  

drivers/gpu/drm/bridge/ite-it6505.c:it6505_enable_audio() {
	...
	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);
	...
}

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for regbe to
be 0 here?

If the hardware measures no audio clock, this register could return 0.

The debug print macro then divides by regbe, which would cause a kernel
panic due to division by zero.

Could we check if regbe is non-zero before using it as a divisor?

[ ... ]

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