| Message ID | 20240223172936.it.875-kees@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] iio: pressure: dlhl60d: Initialize empty DLH bytes | expand |
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.
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>
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...
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
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);
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(-)