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: <24d63ec4-a037-46fd-bbc1-9be2bef34c2b@gmail.com>
Date: Wed, 11 Jun 2025 23:35:21 +0300
From: Cosmin Tanislav <demonsingur@...il.com>
To: Sean Young <sean@...s.org>
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 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@...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.
> 

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
>>