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

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

[Sean Young]

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.

Fixes: 49be1c78d575 ("media: rc: introduce Meson IR TX driver")
Signed-off-by: Sean Young <sean@mess.org>
Cc: stable@vger.kernel.org
---
 drivers/media/rc/meson-ir-tx.c | 54 +++++++++++++++++++++++++++-------
 1 file changed, 44 insertions(+), 10 deletions(-)

diff --git a/drivers/media/rc/meson-ir-tx.c b/drivers/media/rc/meson-ir-tx.c
index 1ab898069e67..a410464ede22 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;
+}
+
+static void meson_irtx_write_mod(struct meson_irtx *ir, unsigned int pulse_cnt,
+				 unsigned int space_cnt)
+{
+	unsigned int cnt = pulse_cnt + space_cnt;
 
 	dev_dbg(ir->dev, "F_mod = %uHz, T_mod = %luns, duty_cycle = %u%%\n",
 		ir->carrier, NSEC_PER_SEC / ir->clk_rate * cnt,
@@ -90,8 +103,10 @@ static void meson_irtx_set_mod(struct meson_irtx *ir)
 	       ir->reg_base + IRB_ADDR1);
 }
 
-static void meson_irtx_setup(struct meson_irtx *ir, unsigned int clk_nr)
+static int meson_irtx_setup(struct meson_irtx *ir, unsigned int clk_nr)
 {
+	unsigned int pulse_cnt, space_cnt;
+
 	/*
 	 * Disable the TX, set modulator clock tick and set initialize
 	 * output to be high. Set up carrier frequency and duty cycle. Then
@@ -100,13 +115,18 @@ static void meson_irtx_setup(struct meson_irtx *ir, unsigned int clk_nr)
 	 */
 	writel(~IRB_ENABLE & (IRB_MOD_CLK(clk_nr) | IRB_INIT_HIGH),
 	       ir->reg_base + IRB_ADDR0);
-	meson_irtx_set_mod(ir);
+	if (!meson_irtx_calc_mod(ir->clk_rate, ir->carrier, ir->duty_cycle,
+				 &pulse_cnt, &space_cnt))
+		return -EINVAL;
+	meson_irtx_write_mod(ir, pulse_cnt, space_cnt);
 	writel(readl(ir->reg_base + IRB_ADDR0) & ~IRB_INIT_HIGH,
 	       ir->reg_base + IRB_ADDR0);
 	writel(IRB_FIFO_IRQ_ENABLE | MIRTX_FIFO_THD,
 	       ir->reg_base + IRB_ADDR3);
 	writel(readl(ir->reg_base + IRB_ADDR0) | IRB_ENABLE,
 	       ir->reg_base + IRB_ADDR0);
+
+	return 0;
 }
 
 static u32 meson_irtx_prepare_pulse(struct meson_irtx *ir, unsigned int time)
@@ -215,12 +235,17 @@ static irqreturn_t meson_irtx_irqhandler(int irq, void *data)
 static int meson_irtx_set_carrier(struct rc_dev *rc, u32 carrier)
 {
 	struct meson_irtx *ir = rc->priv;
+	unsigned int pulse_cnt, space_cnt;
+
+	if (!carrier)
+		return -EINVAL;
 
-	if (carrier == 0)
+	if (!meson_irtx_calc_mod(ir->clk_rate, carrier, ir->duty_cycle,
+				 &pulse_cnt, &space_cnt))
 		return -EINVAL;
 
 	ir->carrier = carrier;
-	meson_irtx_set_mod(ir);
+	meson_irtx_write_mod(ir, pulse_cnt, space_cnt);
 
 	return 0;
 }
@@ -228,9 +253,14 @@ static int meson_irtx_set_carrier(struct rc_dev *rc, u32 carrier)
 static int meson_irtx_set_duty_cycle(struct rc_dev *rc, u32 duty_cycle)
 {
 	struct meson_irtx *ir = rc->priv;
+	unsigned int pulse_cnt, space_cnt;
+
+	if (!meson_irtx_calc_mod(ir->clk_rate, ir->carrier, duty_cycle,
+				 &pulse_cnt, &space_cnt))
+		return -EINVAL;
 
 	ir->duty_cycle = duty_cycle;
-	meson_irtx_set_mod(ir);
+	meson_irtx_write_mod(ir, pulse_cnt, space_cnt);
 
 	return 0;
 }
@@ -337,7 +367,11 @@ static int meson_irtx_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(dev, ret, "modulator clock setup failed\n");
 
-	meson_irtx_setup(ir, clk_nr);
+	ret = meson_irtx_setup(ir, clk_nr);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "carrier %u, duty_cycle %u%% not supported at clk_rate %luHz\n",
+				     ir->carrier, ir->duty_cycle, ir->clk_rate);
 
 	ret = devm_request_irq(dev, irq,
 			       meson_irtx_irqhandler,
-- 
2.55.0


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

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

[sashiko-bot]

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