linux-kernel - Re: [PATCH 06/14] iio: pressure: mprls0025pa: memset rx

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: <20251221182151.288a6da4@jic23-huawei>
Date: Sun, 21 Dec 2025 18:21:51 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Petre Rodan <petre.rodan@...dimension.ro>
Cc: Marcelo Schmitt <marcelo.schmitt1@...il.com>, David Lechner
 <dlechner@...libre.com>, Nuno Sá <nuno.sa@...log.com>, Andy
 Shevchenko <andy@...nel.org>, Andreas Klinger <ak@...klinger.de>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, Jonathan Cameron
 <Jonathan.Cameron@...wei.com>
Subject: Re: [PATCH 06/14] iio: pressure: mprls0025pa: memset rx_buf before
 reading new data

On Sat, 20 Dec 2025 10:25:19 +0200
Petre Rodan <petre.rodan@...dimension.ro> wrote:

> Hello,
> 
> On Sat, Dec 20, 2025 at 01:47:05AM -0300, Marcelo Schmitt wrote:
> > On 12/18, Petre Rodan wrote:  
> > > Zero out input buffer before reading the new conversion.
> > > Perform this operation in core instead of in the bus specific code.
> > > 
> > > Signed-off-by: Petre Rodan <petre.rodan@...dimension.ro>
> > > ---
> > >  drivers/iio/pressure/mprls0025pa.c     | 2 ++
> > >  drivers/iio/pressure/mprls0025pa_i2c.c | 1 -
> > >  2 files changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
> > > index 00b1ff1e50a8..7cc8dd0d8476 100644
> > > --- a/drivers/iio/pressure/mprls0025pa.c
> > > +++ b/drivers/iio/pressure/mprls0025pa.c
> > > @@ -16,6 +16,7 @@
> > >  #include <linux/mod_devicetable.h>
> > >  #include <linux/module.h>
> > >  #include <linux/property.h>
> > > +#include <linux/string.h>
> > >  #include <linux/units.h>
> > >  
> > >  #include <linux/gpio/consumer.h>
> > > @@ -239,6 +240,7 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
> > >  		}
> > >  	}
> > >  
> > > +	memset(data->rx_buf, 0, sizeof(data->rx_buf));  
> > This is unusual and I don't think it's needed for the SPI path. Doesn't the I2C
> > subsystem overwrite the rx buffer with what it reads from the device?  
> 
> I thought it's best practice to ensure that old conversions are not accidentally re-used in case the read operation fell thru the cracks.
> that's exactly why in this particular case the BUSY flag is implemented on the hardware side.
> 
> please tell me how a few byte memset() would be detrimental.

We don't normally do this as old data isn't a potential leak of anything
sensitive.  However in most drivers this only spills out at all as
a result of say a change in configured channels and is normally harmless
as userspace knows to ignore stuff in the gaps anyway.  If there is
another cases here (you mention the busy flag) then add a comment on why
it makes sense. I don't in general want drivers to start doing this as
it is in the fast path and sometimes the memset is non trivial (here it
is probably irrelevant as the buffer is small).

Thanks,

Jonathan

> 
> best regards,
> peter
> 
> > >  	ret = data->ops->read(data, MPR_CMD_NOP, MPR_PKT_NOP_LEN);
> > >  	if (ret < 0)
> > >  		return ret;
> > > diff --git a/drivers/iio/pressure/mprls0025pa_i2c.c b/drivers/iio/pressure/mprls0025pa_i2c.c
> > > index a0bbc6af9283..0fe8cfe0d7e7 100644
> > > --- a/drivers/iio/pressure/mprls0025pa_i2c.c
> > > +++ b/drivers/iio/pressure/mprls0025pa_i2c.c
> > > @@ -25,7 +25,6 @@ static int mpr_i2c_read(struct mpr_data *data, const u8 unused, const u8 cnt)
> > >  	if (cnt > MPR_MEASUREMENT_RD_SIZE)
> > >  		return -EOVERFLOW;
> > >  
> > > -	memset(data->rx_buf, 0, MPR_MEASUREMENT_RD_SIZE);
> > >  	ret = i2c_master_recv(client, data->rx_buf, cnt);
> > >  	if (ret < 0)
> > >  		return ret;  
>