linux-kernel - Re: [PATCH v4 2/2] media: rc: ir-spi: constrain carrier frequency

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aEnifhd1M6oJjy1S@gofer.mess.org>
Date: Wed, 11 Jun 2025 21:09:34 +0100
From: Sean Young <sean@...s.org>
To: Cosmin Tanislav <demonsingur@...il.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/2] media: rc: ir-spi: constrain carrier frequency

On Wed, Jun 11, 2025 at 02:23:44PM +0300, Cosmin Tanislav wrote:
> Carrier frequency is currently unconstrained, allowing the SPI transfer
> to be allocated and filled only for it to be later rejected by the SPI
> controller since the frequency is too large.
> 
> Add a check to constrain the carrier frequency inside
> ir_spi_set_tx_carrier().
> 
> Also, move the number of bits per pulse to a macro since it is not used
> in multiple places.
> 
> Signed-off-by: Cosmin Tanislav <demonsingur@...il.com>
> ---
>  drivers/media/rc/ir-spi.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/ir-spi.c b/drivers/media/rc/ir-spi.c
> index 50e30e2fae22..bf731204c81e 100644
> --- a/drivers/media/rc/ir-spi.c
> +++ b/drivers/media/rc/ir-spi.c
> @@ -21,6 +21,7 @@
>  #define IR_SPI_DRIVER_NAME		"ir-spi"
>  
>  #define IR_SPI_DEFAULT_FREQUENCY	38000
> +#define IR_SPI_BITS_PER_PULSE		16
>  
>  struct ir_spi_data {
>  	u32 freq;
> @@ -70,7 +71,7 @@ static int ir_spi_tx(struct rc_dev *dev, unsigned int *buffer, unsigned int coun
>  
>  	memset(&xfer, 0, sizeof(xfer));
>  
> -	xfer.speed_hz = idata->freq * 16;
> +	xfer.speed_hz = idata->freq * IR_SPI_BITS_PER_PULSE;
>  	xfer.len = len * sizeof(*tx_buf);
>  	xfer.tx_buf = tx_buf;
>  
> @@ -98,6 +99,9 @@ static int ir_spi_set_tx_carrier(struct rc_dev *dev, u32 carrier)
>  	if (!carrier)
>  		return -EINVAL;
>  
> +	if (carrier * IR_SPI_BITS_PER_PULSE > idata->spi->max_speed_hz)
> +		return -EINVAL;

Just a nitpick.

I think carrier * IR_SPI_BITS_PER_PULSE could overflow, and then the check
wouldn't work. It might be better to do:

	if (carrier > idata->spi->max_speed_hz / IR_SPI_BITS_PER_PULSE)

However since IR_SPI_BITS_PER_PULSE is 16, which is just a shift left by 4,
I don't think this can be abused in any useful way.


Sean

> +
>  	idata->freq = carrier;
>  
>  	return 0;
> -- 
> 2.49.0
>