diff mbox series

iio: avoid fortify-string overflow error

Message ID 20240224121140.1883201-1-arnd@kernel.org
State New
Headers
Series iio: avoid fortify-string overflow error |

Commit Message

Arnd Bergmann Feb. 24, 2024, 12:11 p.m. UTC 
  From: Arnd Bergmann <arnd@arndb.de>

The memcpy() call in dlhl60d.c triggers a check with clang-19:

In file included from drivers/iio/pressure/dlhl60d.c:11:
In file included from include/linux/module.h:17:
include/linux/fortify-string.h:553:4: error: call to '__write_overflow_field' declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
  553 |                         __write_overflow_field(p_size_field, size);
      |                         ^

It writes into a two member array from a loop over a linked list
that likely has some indication of having more than two entries.

Add a conditional check there to avoid the overflow.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/iio/pressure/dlhl60d.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Jonathan Cameron Feb. 25, 2024, 12:19 p.m. UTC | #1 
On Sat, 24 Feb 2024 13:11:34 +0100
Arnd Bergmann <arnd@kernel.org> wrote:

> From: Arnd Bergmann <arnd@arndb.de>
> 
> The memcpy() call in dlhl60d.c triggers a check with clang-19:
> 
> In file included from drivers/iio/pressure/dlhl60d.c:11:
> In file included from include/linux/module.h:17:
> include/linux/fortify-string.h:553:4: error: call to '__write_overflow_field' declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
>   553 |                         __write_overflow_field(p_size_field, size);
>       |                         ^
> 
> It writes into a two member array from a loop over a linked list
> that likely has some indication of having more than two entries.
> 
> Add a conditional check there to avoid the overflow.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Hi Arnd,

It's a false positive, but the compiler has no way to tell that only bits
0 and 1 can be set.
https://lore.kernel.org/linux-iio/20240222222335.work.759-kees@kernel.org/
for discussion on why + the missing zero initialization bug Kees noticed whilst
looking at this code.

Kees proposed an alternative way to suppress the warning that I've just applied.
https://lore.kernel.org/linux-iio/20240223172936.it.875-kees@kernel.org/

Your solution also works but leaves the implication of a real path to
overflow the buffer when there isn't one, hence I prefer what Kees had unless
some future version of clang trips over that in which case we can revisit.

Thanks

Jonathan

> ---
>  drivers/iio/pressure/dlhl60d.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/pressure/dlhl60d.c b/drivers/iio/pressure/dlhl60d.c
> index 28c8269ba65d..a43ecda849db 100644
> --- a/drivers/iio/pressure/dlhl60d.c
> +++ b/drivers/iio/pressure/dlhl60d.c
> @@ -262,6 +262,8 @@ static irqreturn_t dlh_trigger_handler(int irq, void *private)
>  			&st->rx_buf[1] + chn * DLH_NUM_DATA_BYTES,
>  			DLH_NUM_DATA_BYTES);
>  		i++;
> +		if (i >= ARRAY_SIZE(tmp_buf))
> +			break;
>  	}
>  
>  	iio_push_to_buffers(indio_dev, tmp_buf);
Arnd Bergmann Feb. 26, 2024, 6:40 a.m. UTC | #2 
On Sun, Feb 25, 2024, at 13:19, Jonathan Cameron wrote:
> On Sat, 24 Feb 2024 13:11:34 +0100 Arnd Bergmann <arnd@kernel.org> wrote:
> It's a false positive, but the compiler has no way to tell that only bits
> 0 and 1 can be set.
> https://lore.kernel.org/linux-iio/20240222222335.work.759-kees@kernel.org/
> for discussion on why + the missing zero initialization bug Kees noticed whilst
> looking at this code.
>
> Kees proposed an alternative way to suppress the warning that I've just applied.
> https://lore.kernel.org/linux-iio/20240223172936.it.875-kees@kernel.org/

Right, that's fine.

> Your solution also works but leaves the implication of a real path to
> overflow the buffer when there isn't one, hence I prefer what Kees had unless
> some future version of clang trips over that in which case we can revisit.

The idea with my patch was to make it obvious to the compiler
that there can't be an overflow, which would ensure the warning
doesn't come back. Kees' version works by avoiding whatever
code path in the compiler trips over the warning, but it's more
likely to come back later if something changes in the compiler
itself, so there is a slight chance that we have it work
around it again.

      Arnd
diff mbox series

Patch

diff --git a/drivers/iio/pressure/dlhl60d.c b/drivers/iio/pressure/dlhl60d.c
index 28c8269ba65d..a43ecda849db 100644
--- a/drivers/iio/pressure/dlhl60d.c
+++ b/drivers/iio/pressure/dlhl60d.c
@@ -262,6 +262,8 @@  static irqreturn_t dlh_trigger_handler(int irq, void *private)
 			&st->rx_buf[1] + chn * DLH_NUM_DATA_BYTES,
 			DLH_NUM_DATA_BYTES);
 		i++;
+		if (i >= ARRAY_SIZE(tmp_buf))
+			break;
 	}
 
 	iio_push_to_buffers(indio_dev, tmp_buf);