linux-kernel - Re: [PATCH v3] media: rc: ir-spi: allocate buffer dynamically

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] [day] [month] [year] [list]

Message-ID: <aEgxRrR0DqNwpdbP@gofer.mess.org>
Date: Tue, 10 Jun 2025 14:21:10 +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 v3] media: rc: ir-spi: allocate buffer dynamically

On Tue, Jun 10, 2025 at 01:15:32PM +0300, Cosmin Tanislav wrote:
> On 6/10/25 12:39 PM, Sean Young wrote:
> > On Mon, Jun 09, 2025 at 02:17:13PM +0300, Cosmin Tanislav wrote:
> > > 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@...il.com>
> > > ---
> > > V3:
> > >   * move the allocation to be done per-TX operation
> > > 
> > > V2:
> > >   * use devm_krealloc_array
> > > 
> > >   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];
> > > +	}
> > 
> > This looks great, thank you.
> > 
> > I do have one concern though. If someone sets a carrier of U32_MAX - 1 then
> > this code could be doing largish allocations, spending too long in kernel
> > space filling them with data and spi can't send it anyway. Actually
> > the kmalloc might fail which doesn't look good in the logs.
> > 
> > We may have to constrain the carrier to something spi can handle.
> > 
> 
> The SPI device has a max_speed_hz, maybe we should check that?
> 
> It seems to be set based on the spi-max-frequency property in
> the device tree node of the SPI device, and uses the max_speed_hz
> of the SPI controller as a fallback.
> 
> Should I add a separate patch that adds a check in
> ir_spi_set_tx_carrier?
> 
> if (carrier * 16 > idata->spi->max_speed_hz)
> 	return -EINVAL.
> 
> Something along these lines.

That's perfect. The 16 will need a comment or a #define


Sean