[PATCH v2] iio: pressure: dlhl60d: Initialize empty DLH bytes

[PATCH v2] iio: pressure: dlhl60d: Initialize empty DLH bytes

[Kees Cook]

3 bytes were being read but 4 were being written. Explicitly initialize
the unused bytes to 0 and refactor the loop to use direct array
indexing, which appears to silence a Clang false positive warning[1].

Link: https://github.com/ClangBuiltLinux/linux/issues/2000 [1]
Fixes: ac78c6aa4a5d ("iio: pressure: Add driver for DLH pressure sensors")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: "Lars-Peter Clausen" <lars@metafoo.de>
Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: "Andy Shevchenko" <andriy.shevchenko@linux.intel.com>
Cc: "Nuno Sá" <nuno.sa@analog.com>
Cc: linux-iio@vger.kernel.org
Cc: "Nathan Chancellor" <nathan@kernel.org>
Cc: "Nick Desaulniers" <ndesaulniers@google.com>
Cc: "Bill Wendling" <morbo@google.com>
Cc: "Justin Stitt" <justinstitt@google.com>
Cc: llvm@lists.linux.dev
 v2: drop comments, array expansion, and WARN. refactor loop.
 v1: https://lore.kernel.org/linux-hardening/20240222222335.work.759-kees@kernel.org/
---
 drivers/iio/pressure/dlhl60d.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/pressure/dlhl60d.c b/drivers/iio/pressure/dlhl60d.c
index 28c8269ba65d..0bba4c5a8d40 100644
--- a/drivers/iio/pressure/dlhl60d.c
+++ b/drivers/iio/pressure/dlhl60d.c
@@ -250,18 +250,17 @@ static irqreturn_t dlh_trigger_handler(int irq, void *private)
 	struct dlh_state *st = iio_priv(indio_dev);
 	int ret;
 	unsigned int chn, i = 0;
-	__be32 tmp_buf[2];
+	__be32 tmp_buf[2] = { };
 
 	ret = dlh_start_capture_and_read(st);
 	if (ret)
 		goto out;
 
 	for_each_set_bit(chn, indio_dev->active_scan_mask,
-		indio_dev->masklength) {
-		memcpy(tmp_buf + i,
+			 indio_dev->masklength) {
+		memcpy(&tmp_buf[i++],
 			&st->rx_buf[1] + chn * DLH_NUM_DATA_BYTES,
 			DLH_NUM_DATA_BYTES);
-		i++;
 	}
 
 	iio_push_to_buffers(indio_dev, tmp_buf);
-- 
2.34.1

Re: [PATCH v2] iio: pressure: dlhl60d: Initialize empty DLH bytes

[Andy Shevchenko]

On Fri, Feb 23, 2024 at 09:29:39AM -0800, Kees Cook wrote:
> 3 bytes were being read but 4 were being written. Explicitly initialize
> the unused bytes to 0 and refactor the loop to use direct array
> indexing, which appears to silence a Clang false positive warning[1].

...

>  	for_each_set_bit(chn, indio_dev->active_scan_mask,
> -		indio_dev->masklength) {
> -		memcpy(tmp_buf + i,
> +			 indio_dev->masklength) {
> +		memcpy(&tmp_buf[i++],
>  			&st->rx_buf[1] + chn * DLH_NUM_DATA_BYTES,
>  			DLH_NUM_DATA_BYTES);
> -		i++;
>  	}

Not that I'm against the changes, but they (in accordance with the commit
message) are irrelevant to this fix. I prefer fixes to be more focused on
the real issues.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: pressure: dlhl60d: Initialize empty DLH bytes

[Andy Shevchenko]

On Fri, Feb 23, 2024 at 07:47:36PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 23, 2024 at 09:29:39AM -0800, Kees Cook wrote:
> > 3 bytes were being read but 4 were being written. Explicitly initialize
> > the unused bytes to 0 and refactor the loop to use direct array
> > indexing, which appears to silence a Clang false positive warning[1].

...

> >  	for_each_set_bit(chn, indio_dev->active_scan_mask,
> > -		indio_dev->masklength) {
> > -		memcpy(tmp_buf + i,
> > +			 indio_dev->masklength) {
> > +		memcpy(&tmp_buf[i++],
> >  			&st->rx_buf[1] + chn * DLH_NUM_DATA_BYTES,
> >  			DLH_NUM_DATA_BYTES);
> > -		i++;
> >  	}
> 
> Not that I'm against the changes, but they (in accordance with the commit
> message) are irrelevant to this fix. I prefer fixes to be more focused on
> the real issues.

Ah, sorry, there are two changes here:
- indentation (which is indeed irrelevant)
- and indexing, which seems the needed one.

Whatever,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: pressure: dlhl60d: Initialize empty DLH bytes

[Kees Cook]

On Fri, Feb 23, 2024 at 07:47:36PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 23, 2024 at 09:29:39AM -0800, Kees Cook wrote:
> > 3 bytes were being read but 4 were being written. Explicitly initialize
> > the unused bytes to 0 and refactor the loop to use direct array
> > indexing, which appears to silence a Clang false positive warning[1].
> 
> ...
> 
> >  	for_each_set_bit(chn, indio_dev->active_scan_mask,
> > -		indio_dev->masklength) {
> > -		memcpy(tmp_buf + i,
> > +			 indio_dev->masklength) {
> > +		memcpy(&tmp_buf[i++],
> >  			&st->rx_buf[1] + chn * DLH_NUM_DATA_BYTES,
> >  			DLH_NUM_DATA_BYTES);
> > -		i++;
> >  	}
> 
> Not that I'm against the changes, but they (in accordance with the commit
> message) are irrelevant to this fix. I prefer fixes to be more focused on
> the real issues.

Jonathan, let me know if you'd prefer I split this patch...

-- 
Kees Cook

Re: [PATCH v2] iio: pressure: dlhl60d: Initialize empty DLH bytes

[Jonathan Cameron]

On Fri, 23 Feb 2024 09:50:10 -0800
Kees Cook <keescook@chromium.org> wrote:

> On Fri, Feb 23, 2024 at 07:47:36PM +0200, Andy Shevchenko wrote:
> > On Fri, Feb 23, 2024 at 09:29:39AM -0800, Kees Cook wrote:  
> > > 3 bytes were being read but 4 were being written. Explicitly initialize
> > > the unused bytes to 0 and refactor the loop to use direct array
> > > indexing, which appears to silence a Clang false positive warning[1].  
> > 
> > ...
> >   
> > >  	for_each_set_bit(chn, indio_dev->active_scan_mask,
> > > -		indio_dev->masklength) {
> > > -		memcpy(tmp_buf + i,
> > > +			 indio_dev->masklength) {
> > > +		memcpy(&tmp_buf[i++],
> > >  			&st->rx_buf[1] + chn * DLH_NUM_DATA_BYTES,
> > >  			DLH_NUM_DATA_BYTES);
> > > -		i++;
> > >  	}  
> > 
> > Not that I'm against the changes, but they (in accordance with the commit
> > message) are irrelevant to this fix. I prefer fixes to be more focused on
> > the real issues.  
> 
> Jonathan, let me know if you'd prefer I split this patch...
> 
Andy is strictly speaking correct that the indent should be separate patch
but meh - not worth the time to split that out + the change makes the
fixed code itself easier to read. 

I added a tiny comment to say it the indent tidying up was incorporated
so the fixed code was more readable.

Applied to the fixes-togreg branch of iio.git and marked for stable.

Given timing this may well go in during the merge window rather than
before.

Jonathan