[PATCH v4 0/2] media: rc: ir-spi: allocate buffer dynamically

[PATCH v4 0/2] media: rc: ir-spi: allocate buffer dynamically

[Cosmin Tanislav]

Replace the static transmit buffer with a dynamically allocated one,
removing the limit imposed on the number of pulses to transmit.

Add a check to constrain the carrier frequency inside
ir_spi_set_tx_carrier().

V4:
 * add separate patch to constrain the carrier frequency

V3:
 * move the allocation to be done per-TX operation

V2:
 * use devm_krealloc_array

Cosmin Tanislav (2):
  media: rc: ir-spi: allocate buffer dynamically
  media: rc: ir-spi: constrain carrier frequency

 drivers/media/rc/ir-spi.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

-- 
2.49.0

[PATCH v4 1/2] media: rc: ir-spi: allocate buffer dynamically

[Cosmin Tanislav]

Replace the static transmit buffer with a dynamically allocated one,
removing the limit imposed on the number of pulses to transmit.

Calculate the number of pulses for each duration in the received buffer
ahead of time, while also adding up the total pulses, to be able to
allocate a buffer that perfectly fits the total number of pulses, then
populate it.

Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
---
 drivers/media/rc/ir-spi.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/media/rc/ir-spi.c b/drivers/media/rc/ir-spi.c
index 8fc8e496e6aa..50e30e2fae22 100644
--- a/drivers/media/rc/ir-spi.c
+++ b/drivers/media/rc/ir-spi.c
@@ -21,13 +21,11 @@
 #define IR_SPI_DRIVER_NAME		"ir-spi"
 
 #define IR_SPI_DEFAULT_FREQUENCY	38000
-#define IR_SPI_MAX_BUFSIZE		 4096
 
 struct ir_spi_data {
 	u32 freq;
 	bool negated;
 
-	u16 tx_buf[IR_SPI_MAX_BUFSIZE];
 	u16 pulse;
 	u16 space;
 
@@ -43,37 +41,42 @@ static int ir_spi_tx(struct rc_dev *dev, unsigned int *buffer, unsigned int coun
 	unsigned int len = 0;
 	struct ir_spi_data *idata = dev->priv;
 	struct spi_transfer xfer;
+	u16 *tx_buf;
 
 	/* convert the pulse/space signal to raw binary signal */
 	for (i = 0; i < count; i++) {
-		unsigned int periods;
+		buffer[i] = DIV_ROUND_CLOSEST(buffer[i] * idata->freq, 1000000);
+		len += buffer[i];
+	}
+
+	tx_buf = kmalloc_array(len, sizeof(*tx_buf), GFP_KERNEL);
+	if (!tx_buf)
+		return -ENOMEM;
+
+	len = 0;
+	for (i = 0; i < count; i++) {
 		int j;
 		u16 val;
 
-		periods = DIV_ROUND_CLOSEST(buffer[i] * idata->freq, 1000000);
-
-		if (len + periods >= IR_SPI_MAX_BUFSIZE)
-			return -EINVAL;
-
 		/*
 		 * The first value in buffer is a pulse, so that 0, 2, 4, ...
 		 * contain a pulse duration. On the contrary, 1, 3, 5, ...
 		 * contain a space duration.
 		 */
 		val = (i % 2) ? idata->space : idata->pulse;
-		for (j = 0; j < periods; j++)
-			idata->tx_buf[len++] = val;
+		for (j = 0; j < buffer[i]; j++)
+			tx_buf[len++] = val;
 	}
 
 	memset(&xfer, 0, sizeof(xfer));
 
 	xfer.speed_hz = idata->freq * 16;
-	xfer.len = len * sizeof(*idata->tx_buf);
-	xfer.tx_buf = idata->tx_buf;
+	xfer.len = len * sizeof(*tx_buf);
+	xfer.tx_buf = tx_buf;
 
 	ret = regulator_enable(idata->regulator);
 	if (ret)
-		return ret;
+		goto err_free_tx_buf;
 
 	ret = spi_sync_transfer(idata->spi, &xfer, 1);
 	if (ret)
@@ -81,6 +84,10 @@ static int ir_spi_tx(struct rc_dev *dev, unsigned int *buffer, unsigned int coun
 
 	regulator_disable(idata->regulator);
 
+err_free_tx_buf:
+
+	kfree(tx_buf);
+
 	return ret ? ret : count;
 }
 
-- 
2.49.0

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

[Cosmin Tanislav]

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@gmail.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;
+
 	idata->freq = carrier;
 
 	return 0;
-- 
2.49.0

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

[Sean Young]

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@gmail.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
> 

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

[Cosmin Tanislav]

On 6/11/25 11:09 PM, Sean Young wrote:
> 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@gmail.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.
> 

I have another concern regarding overflow, inside ir_spi_tx().

DIV_ROUND_CLOSEST() is called with buffer[i] * idata->freq and 1000000.
buffer[i] comes from userspace, it's the number of microseconds for this
pulse. It's unsigned int. lirc core already checks that each element
is not bigger than 500000 microseconds. Issue is, at 500000, it would
take a carrier frequency as low as 8590 to overflow the unsigned int.

Maybe it would make sense to switch this one to mult_frac()? But we
would lose rounding.

mult_frac(buffer[i], idata->freq, 1000000)

Optionally, we could cast buffer[i] to u64/unsigned long long, and use
DIV_ROUND_CLOSEST_ULL.

DIV_ROUND_CLOSEST_ULL((u64)buffer[i] * idata->freq, 1000000)

Let me know what you think.

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

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

[Sean Young]

On Wed, Jun 11, 2025 at 11:35:21PM +0300, Cosmin Tanislav wrote:
> On 6/11/25 11:09 PM, Sean Young wrote:
> > 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@gmail.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.
> > 
> 
> I have another concern regarding overflow, inside ir_spi_tx().
> 
> DIV_ROUND_CLOSEST() is called with buffer[i] * idata->freq and 1000000.
> buffer[i] comes from userspace, it's the number of microseconds for this
> pulse. It's unsigned int. lirc core already checks that each element
> is not bigger than 500000 microseconds. Issue is, at 500000, it would
> take a carrier frequency as low as 8590 to overflow the unsigned int.

Interesting, you are right.

> Maybe it would make sense to switch this one to mult_frac()? But we
> would lose rounding.
> 
> mult_frac(buffer[i], idata->freq, 1000000)
> 
> Optionally, we could cast buffer[i] to u64/unsigned long long, and use
> DIV_ROUND_CLOSEST_ULL.
> 
> DIV_ROUND_CLOSEST_ULL((u64)buffer[i] * idata->freq, 1000000)
> 
> Let me know what you think.

I've given it some thought and I'm not sure there is a better solution. It's
an edge case of course, but we should deal with it correctly.

Nice catch, solution looks good.


Sean

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

[Sean Young]

On Thu, Jun 12, 2025 at 09:02:59PM +0100, Sean Young wrote:
> On Wed, Jun 11, 2025 at 11:35:21PM +0300, Cosmin Tanislav wrote:
> > On 6/11/25 11:09 PM, Sean Young wrote:
> > > 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@gmail.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.
> > > 
> > 
> > I have another concern regarding overflow, inside ir_spi_tx().
> > 
> > DIV_ROUND_CLOSEST() is called with buffer[i] * idata->freq and 1000000.
> > buffer[i] comes from userspace, it's the number of microseconds for this
> > pulse. It's unsigned int. lirc core already checks that each element
> > is not bigger than 500000 microseconds. Issue is, at 500000, it would
> > take a carrier frequency as low as 8590 to overflow the unsigned int.
> 
> Interesting, you are right.
> 
> > Maybe it would make sense to switch this one to mult_frac()? But we
> > would lose rounding.
> > 
> > mult_frac(buffer[i], idata->freq, 1000000)
> > 
> > Optionally, we could cast buffer[i] to u64/unsigned long long, and use
> > DIV_ROUND_CLOSEST_ULL.
> > 
> > DIV_ROUND_CLOSEST_ULL((u64)buffer[i] * idata->freq, 1000000)
> > 
> > Let me know what you think.
> 
> I've given it some thought and I'm not sure there is a better solution. It's
> an edge case of course, but we should deal with it correctly.

Actually could we use check_mul_overflow() for this?

Just an idea.


Sean

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

[Cosmin Tanislav]

c

On 6/12/25 11:10 PM, Sean Young wrote:
> On Thu, Jun 12, 2025 at 09:02:59PM +0100, Sean Young wrote:
>> On Wed, Jun 11, 2025 at 11:35:21PM +0300, Cosmin Tanislav wrote:
>>> On 6/11/25 11:09 PM, Sean Young wrote:
>>>> 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@gmail.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.
>>>>
>>>
>>> I have another concern regarding overflow, inside ir_spi_tx().
>>>
>>> DIV_ROUND_CLOSEST() is called with buffer[i] * idata->freq and 1000000.
>>> buffer[i] comes from userspace, it's the number of microseconds for this
>>> pulse. It's unsigned int. lirc core already checks that each element
>>> is not bigger than 500000 microseconds. Issue is, at 500000, it would
>>> take a carrier frequency as low as 8590 to overflow the unsigned int.
>>
>> Interesting, you are right.
>>
>>> Maybe it would make sense to switch this one to mult_frac()? But we
>>> would lose rounding.
>>>
>>> mult_frac(buffer[i], idata->freq, 1000000)
>>>
>>> Optionally, we could cast buffer[i] to u64/unsigned long long, and use
>>> DIV_ROUND_CLOSEST_ULL.
>>>
>>> DIV_ROUND_CLOSEST_ULL((u64)buffer[i] * idata->freq, 1000000)
>>>
>>> Let me know what you think.
>>
>> I've given it some thought and I'm not sure there is a better solution. It's
>> an edge case of course, but we should deal with it correctly.
> 
> Actually could we use check_mul_overflow() for this?
> 

I think we're better off using DIV_ROUND_CLOSEST_ULL(), since after the
multiplication, there's a division by 1000000, which might bring us back
in 32-bit territory, even if the multiplication overflowed. If we use
check_mul_overflow(), we would just invalidate a case that would have
worked fine.

> Just an idea.
> 
> 
> Sean

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

[Sean Young]

On Thu, Jun 12, 2025 at 11:20:28PM +0300, Cosmin Tanislav wrote:
> On 6/12/25 11:10 PM, Sean Young wrote:
> > On Thu, Jun 12, 2025 at 09:02:59PM +0100, Sean Young wrote:
> > > On Wed, Jun 11, 2025 at 11:35:21PM +0300, Cosmin Tanislav wrote:
> > > > On 6/11/25 11:09 PM, Sean Young wrote:
> > > > > 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@gmail.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.
> > > > > 
> > > > 
> > > > I have another concern regarding overflow, inside ir_spi_tx().
> > > > 
> > > > DIV_ROUND_CLOSEST() is called with buffer[i] * idata->freq and 1000000.
> > > > buffer[i] comes from userspace, it's the number of microseconds for this
> > > > pulse. It's unsigned int. lirc core already checks that each element
> > > > is not bigger than 500000 microseconds. Issue is, at 500000, it would
> > > > take a carrier frequency as low as 8590 to overflow the unsigned int.
> > > 
> > > Interesting, you are right.
> > > 
> > > > Maybe it would make sense to switch this one to mult_frac()? But we
> > > > would lose rounding.
> > > > 
> > > > mult_frac(buffer[i], idata->freq, 1000000)
> > > > 
> > > > Optionally, we could cast buffer[i] to u64/unsigned long long, and use
> > > > DIV_ROUND_CLOSEST_ULL.
> > > > 
> > > > DIV_ROUND_CLOSEST_ULL((u64)buffer[i] * idata->freq, 1000000)
> > > > 
> > > > Let me know what you think.
> > > 
> > > I've given it some thought and I'm not sure there is a better solution. It's
> > > an edge case of course, but we should deal with it correctly.
> > 
> > Actually could we use check_mul_overflow() for this?
> > 
> 
> I think we're better off using DIV_ROUND_CLOSEST_ULL(), since after the
> multiplication, there's a division by 1000000, which might bring us back
> in 32-bit territory, even if the multiplication overflowed. If we use
> check_mul_overflow(), we would just invalidate a case that would have
> worked fine.

I don't have a strong opinion on this, but in the current code the overflow
is not detected and garbage is sent, right?


Sean

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

[Cosmin Tanislav]

On 6/13/25 12:30 PM, Sean Young wrote:
> On Thu, Jun 12, 2025 at 11:20:28PM +0300, Cosmin Tanislav wrote:
>> On 6/12/25 11:10 PM, Sean Young wrote:
>>> On Thu, Jun 12, 2025 at 09:02:59PM +0100, Sean Young wrote:
>>>> On Wed, Jun 11, 2025 at 11:35:21PM +0300, Cosmin Tanislav wrote:
>>>>> On 6/11/25 11:09 PM, Sean Young wrote:
>>>>>> 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@gmail.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.
>>>>>>
>>>>>
>>>>> I have another concern regarding overflow, inside ir_spi_tx().
>>>>>
>>>>> DIV_ROUND_CLOSEST() is called with buffer[i] * idata->freq and 1000000.
>>>>> buffer[i] comes from userspace, it's the number of microseconds for this
>>>>> pulse. It's unsigned int. lirc core already checks that each element
>>>>> is not bigger than 500000 microseconds. Issue is, at 500000, it would
>>>>> take a carrier frequency as low as 8590 to overflow the unsigned int.
>>>>
>>>> Interesting, you are right.
>>>>
>>>>> Maybe it would make sense to switch this one to mult_frac()? But we
>>>>> would lose rounding.
>>>>>
>>>>> mult_frac(buffer[i], idata->freq, 1000000)
>>>>>
>>>>> Optionally, we could cast buffer[i] to u64/unsigned long long, and use
>>>>> DIV_ROUND_CLOSEST_ULL.
>>>>>
>>>>> DIV_ROUND_CLOSEST_ULL((u64)buffer[i] * idata->freq, 1000000)
>>>>>
>>>>> Let me know what you think.
>>>>
>>>> I've given it some thought and I'm not sure there is a better solution. It's
>>>> an edge case of course, but we should deal with it correctly.
>>>
>>> Actually could we use check_mul_overflow() for this?
>>>
>>
>> I think we're better off using DIV_ROUND_CLOSEST_ULL(), since after the
>> multiplication, there's a division by 1000000, which might bring us back
>> in 32-bit territory, even if the multiplication overflowed. If we use
>> check_mul_overflow(), we would just invalidate a case that would have
>> worked fine.
> 
> I don't have a strong opinion on this, but in the current code the overflow
> is not detected and garbage is sent, right?
> 

Yes, that's the current situation. idata->freq can be at most 8590
knowing buffer[i] is limited to 500000:

0xFFFFFFFF / 500000 ~= 8590

If we switch to u64 for the multiplication, idata->freq can be larger
than the u32 max value without overflowing the multiplication:

0xFFFFFFFFFFFFFFFF / 500000 ~= 36893488147420 > 4294967295 (u32 limit)

Now, knowing that buffer[i] is max 500000, and freq is u32,
the max value of the whole DIV_ROUND_CLOSEST_ULL() call would be:

500000 * 0xFFFFFFFF / 1000000 ~= 2147483648

Which fits fine in u32, seeing how 500000 is half of 1000000.

> 
> Sean