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 <[email protected]>
---
Cc: Jonathan Cameron <[email protected]>
Cc: "Lars-Peter Clausen" <[email protected]>
Cc: "Uwe Kleine-König" <[email protected]>
Cc: "Andy Shevchenko" <[email protected]>
Cc: "Nuno Sá" <[email protected]>
Cc: [email protected]
Cc: "Nathan Chancellor" <[email protected]>
Cc: "Nick Desaulniers" <[email protected]>
Cc: "Bill Wendling" <[email protected]>
Cc: "Justin Stitt" <[email protected]>
Cc: [email protected]
v2: drop comments, array expansion, and WARN. refactor loop.
v1: https://lore.kernel.org/linux-hardening/[email protected]/
---
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
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
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 <[email protected]>
--
With Best Regards,
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.
Jonathan, let me know if you'd prefer I split this patch...
--
Kees Cook
On Fri, 23 Feb 2024 09:50:10 -0800
Kees Cook <[email protected]> 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