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: <aUjd5AJHohyW_kXM@lipo.home.arpa>
Date: Mon, 22 Dec 2025 07:57:56 +0200
From: Petre Rodan <petre.rodan@...dimension.ro>
To: Jonathan Cameron <jic23@...nel.org>
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


hello Jonathan,

thank you for the review.

On Sun, Dec 21, 2025 at 06:21:51PM +0000, Jonathan Cameron wrote:
> 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.

from my point of view as someone writing drivers for chemistry lab instruments, stale readings are to be avoided at all costs.
it's not about leaking sensitive data, it's about providing a warning sign if the read operation fails silently.

as an extreme (but fictional) example, a pilot looking at an altimeter would immediately recognize that something is wrong with it's pitot tube if it's giving out an off-scale static reading. if instead the output is believable (which would be the case when older readings are repeated due to an uncaught intermittent read error) then there would be some uncertainty and he would not know to definitely ignore the output of this particular instrument and trust another one instead.

the same logic applies to any instrument in a lab setting. 
a digital titration system that mixes multiple reagents needs to rely on fresh conversions to know when to stop a process. some advanced sensors even provide an incrementing conversion counter, others simply signal that a measurement is ongoing/not fresh via a BUSY flag and these are designed so that the driver can avoid a stale reading.

getting back to my driver, some pressure sensor series have a latch-up sensitivity and they misbehave during reads in various ways under certain conditions. I understand that you say that silent fails are unlikely but I'd like to keep the memset, for peace of mind.

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

-- 
petre rodan

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)