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

[Cosmin Tanislav <demonsingur@gmail.com>]

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