Re: [PATCH v4 19/19] media: meson-ir-tx: Validate carrier and duty_cycle

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch claims to fix a divide-by-zero error for carrier frequencies larger than USEC_PER_SEC, but the implemented bounds check mathematically fails to prevent it.
--

commit e3f11b02aad6109d1dcf403fdfe01c4cc2db70e1
Author: Sean Young <sean@mess.org>

media: meson-ir-tx: Validate carrier and duty_cycle

It is possible to set a combination of carrier and duty cycle that do
not produce a correct signal. In addition, setting a carrier larger
than USEC_PER_SEC will result in a divide by zero in
meson_irtx_prepare_pulse() during transmit.

> diff --git a/drivers/media/rc/meson-ir-tx.c b/drivers/media/rc/meson-ir-tx.c
> index 1ab898069e67f..a410464ede224 100644
> --- a/drivers/media/rc/meson-ir-tx.c
> +++ b/drivers/media/rc/meson-ir-tx.c

[ ... ]

> @@ -76,11 +76,24 @@ struct meson_irtx {
>  	unsigned long clk_rate;
>  };
>  
> -static void meson_irtx_set_mod(struct meson_irtx *ir)
> +static bool meson_irtx_calc_mod(unsigned long clk_rate, u32 carrier,
> +				u32 duty_cycle, unsigned int *pulse_cnt,
> +				unsigned int *space_cnt)
>  {
> -	unsigned int cnt = DIV_ROUND_CLOSEST(ir->clk_rate, ir->carrier);
> -	unsigned int pulse_cnt = DIV_ROUND_CLOSEST(cnt * ir->duty_cycle, 100);
> -	unsigned int space_cnt = cnt - pulse_cnt;
> +	unsigned int cnt;
> +
> +	cnt = DIV_ROUND_CLOSEST(clk_rate, carrier);
> +	*pulse_cnt = DIV_ROUND_CLOSEST(cnt * duty_cycle, 100);
> +	*space_cnt = cnt - *pulse_cnt;
> +
> +	return *pulse_cnt >= 1 && *pulse_cnt <= 65536 &&
> +	       *space_cnt >= 1 && *space_cnt <= 65536;
> +}

[Severity: Medium]
Does this validation check adequately prevent the divide-by-zero condition
mentioned in the commit message?

For example, if the carrier frequency is set to 3,000,000 Hz and the clock
rate is 8,000,000 Hz, cnt evaluates to 3. Depending on the duty cycle,
pulse_cnt and space_cnt could be valid (e.g., 2 and 1), allowing this
function to return true despite the carrier being larger than USEC_PER_SEC.

Because of this, callers like meson_irtx_set_carrier() can still accept
these excessively high carrier frequencies, allowing tb_us to drop to 0
in meson_irtx_prepare_pulse():

	unsigned int tb_us = DIV_ROUND_CLOSEST(USEC_PER_SEC, ir->carrier);
	delay = (DIV_ROUND_CLOSEST(time, tb_us) - 1) & IRB_DELAY_MASK;

While a pre-existing bounds check in meson_irtx_check_buf() seems to
coincidentally reject transmissions when carrier > USEC_PER_SEC (since its
calculated max_tb_us becomes 0), should this new logic also explicitly
check for carrier <= USEC_PER_SEC to fully enforce the constraint stated
in the commit message?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788882189.git.sean@mess.org?part=19

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic