1. Patchew
  2. linux
  3. [PATCH v4 0/2] media: rc: ir-spi: allocate buffer dynamically
  4. View patch

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

Cosmin Tanislav posted 2 patches 4 months ago
There is a newer version of this series
[PATCH v4 2/2] media: rc: ir-spi: constrain carrier frequency
Posted by Cosmin Tanislav 4 months ago 
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
Posted by Sean Young 4 months ago 
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
Posted by Cosmin Tanislav 4 months ago 

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
Posted by Sean Young 4 months ago 
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
Posted by Sean Young 4 months ago 
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
Posted by Cosmin Tanislav 4 months ago 
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
Posted by Sean Young 3 months, 4 weeks ago 
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
Posted by Cosmin Tanislav 3 months, 4 weeks ago 

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.