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: <aUlQTxtWo0VUi4Yh@debian-BULLSEYE-live-builder-AMD64>
Date: Mon, 22 Dec 2025 11:06:07 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: Petre Rodan <petre.rodan@...dimension.ro>
Cc: Jonathan Cameron <jic23@...nel.org>,
	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

Hi Petre,

On 12/22, Petre Rodan wrote:
> 
> 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.

I agree with you that old conversions should not be accidentally re-used nor
errors silently be ignored. But, to me, memset the read buffer to zero looks
like we don't trust the underlying I2C and SPI layers. In that case, we should
fix data read in those subsystems (if there is anyhting be fixed there).
Though probably unlikely scenario to happen, how would one trust the sensor
reading in a scenario where the extected measurement would be close to zero.

My suggestion is to look for a way of ensuring the transfer timing requirements
specified in data sheet page 18 [1]. See the dummy delay transfer suggestion to
patch 09.

[1]: https://4donline.ihs.com/images/VipMasterIC/IC/HWSC/HWSC-S-A0016036563/HWSC-S-A0016036563-1.pdf?hkey=CECEF36DEECDED6468708AAF2E19C0C6

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