* [PATCH] auxdisplay: ht16k33: don't access uninitialized data
@ 2017-03-28 10:11 Arnd Bergmann
2017-03-29 7:31 ` Dmitry Torokhov
2017-03-29 8:07 ` Robin van der Gracht
0 siblings, 2 replies; 3+ messages in thread
From: Arnd Bergmann @ 2017-03-28 10:11 UTC (permalink / raw)
To: Robin van der Gracht, Miguel Ojeda Sandonis
Cc: Arnd Bergmann, Greg Kroah-Hartman, Dmitry Torokhov, Rob Herring,
Linus Walleij, linux-kernel
gcc-7.0.1 points out that we copy uninitialized data from the stack
into a per-device structure:
drivers/auxdisplay/ht16k33.c: In function 'ht16k33_keypad_irq_thread':
arch/x86/include/asm/string_32.h:78:16: error: 'new_state' may be used uninitialized in this function [-Werror=maybe-uninitialized]
arch/x86/include/asm/string_32.h:79:22: error: '*((void *)&new_state+4)' may be used uninitialized in this function [-Werror=maybe-uninitialized]
The access is harmless because we never read the data, but we are better
off not doing this, so this changes the code to only copy the data
that was actually initialized. To make sure we don't overflow the
stack with an incorrect DT, we also need to add a sanity checkin the
probe function.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/auxdisplay/ht16k33.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index f66b45b235b0..ba6370974574 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -278,7 +278,7 @@ static bool ht16k33_keypad_scan(struct ht16k33_keypad *keypad)
}
}
input_sync(keypad->dev);
- memcpy(keypad->last_key_state, new_state, sizeof(new_state));
+ memcpy(keypad->last_key_state, new_state, sizeof(u16) * keypad->cols);
return pressed;
}
@@ -353,6 +353,12 @@ static int ht16k33_keypad_probe(struct i2c_client *client,
err = matrix_keypad_parse_of_params(&client->dev, &rows, &cols);
if (err)
return err;
+ if (rows > HT16K33_MATRIX_KEYPAD_MAX_ROWS ||
+ cols > HT16K33_MATRIX_KEYPAD_MAX_COLS) {
+ dev_err(&client->dev, "%u rows or %u cols out of range in DT\n",
+ rows, cols);
+ return -ERANGE;
+ }
keypad->rows = rows;
keypad->cols = cols;
--
2.9.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] auxdisplay: ht16k33: don't access uninitialized data
2017-03-28 10:11 [PATCH] auxdisplay: ht16k33: don't access uninitialized data Arnd Bergmann
@ 2017-03-29 7:31 ` Dmitry Torokhov
2017-03-29 8:07 ` Robin van der Gracht
1 sibling, 0 replies; 3+ messages in thread
From: Dmitry Torokhov @ 2017-03-29 7:31 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Robin van der Gracht, Miguel Ojeda Sandonis, Greg Kroah-Hartman,
Rob Herring, Linus Walleij, linux-kernel
On Tue, Mar 28, 2017 at 12:11:49PM +0200, Arnd Bergmann wrote:
> gcc-7.0.1 points out that we copy uninitialized data from the stack
> into a per-device structure:
>
> drivers/auxdisplay/ht16k33.c: In function 'ht16k33_keypad_irq_thread':
> arch/x86/include/asm/string_32.h:78:16: error: 'new_state' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> arch/x86/include/asm/string_32.h:79:22: error: '*((void *)&new_state+4)' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> The access is harmless because we never read the data, but we are better
> off not doing this, so this changes the code to only copy the data
> that was actually initialized. To make sure we don't overflow the
> stack with an incorrect DT, we also need to add a sanity checkin the
> probe function.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/auxdisplay/ht16k33.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
> index f66b45b235b0..ba6370974574 100644
> --- a/drivers/auxdisplay/ht16k33.c
> +++ b/drivers/auxdisplay/ht16k33.c
> @@ -278,7 +278,7 @@ static bool ht16k33_keypad_scan(struct ht16k33_keypad *keypad)
> }
> }
> input_sync(keypad->dev);
> - memcpy(keypad->last_key_state, new_state, sizeof(new_state));
> + memcpy(keypad->last_key_state, new_state, sizeof(u16) * keypad->cols);
>
> return pressed;
> }
> @@ -353,6 +353,12 @@ static int ht16k33_keypad_probe(struct i2c_client *client,
> err = matrix_keypad_parse_of_params(&client->dev, &rows, &cols);
> if (err)
> return err;
> + if (rows > HT16K33_MATRIX_KEYPAD_MAX_ROWS ||
> + cols > HT16K33_MATRIX_KEYPAD_MAX_COLS) {
> + dev_err(&client->dev, "%u rows or %u cols out of range in DT\n",
> + rows, cols);
> + return -ERANGE;
> + }
>
> keypad->rows = rows;
> keypad->cols = cols;
> --
> 2.9.0
>
--
Dmitry
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] auxdisplay: ht16k33: don't access uninitialized data
2017-03-28 10:11 [PATCH] auxdisplay: ht16k33: don't access uninitialized data Arnd Bergmann
2017-03-29 7:31 ` Dmitry Torokhov
@ 2017-03-29 8:07 ` Robin van der Gracht
1 sibling, 0 replies; 3+ messages in thread
From: Robin van der Gracht @ 2017-03-29 8:07 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Miguel Ojeda Sandonis, Greg Kroah-Hartman, Dmitry Torokhov,
Rob Herring, Linus Walleij, linux-kernel
On Tue, 28 Mar 2017 12:11:49 +0200
Arnd Bergmann <arnd@arndb.de> wrote:
> gcc-7.0.1 points out that we copy uninitialized data from the stack
> into a per-device structure:
>
> drivers/auxdisplay/ht16k33.c: In function 'ht16k33_keypad_irq_thread':
> arch/x86/include/asm/string_32.h:78:16: error: 'new_state' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> arch/x86/include/asm/string_32.h:79:22: error: '*((void *)&new_state+4)' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> The access is harmless because we never read the data, but we are better
> off not doing this, so this changes the code to only copy the data
> that was actually initialized. To make sure we don't overflow the
> stack with an incorrect DT, we also need to add a sanity checkin the
> probe function.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Robin van der Gracht <robin@protonic.nl>
> ---
> drivers/auxdisplay/ht16k33.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
> index f66b45b235b0..ba6370974574 100644
> --- a/drivers/auxdisplay/ht16k33.c
> +++ b/drivers/auxdisplay/ht16k33.c
> @@ -278,7 +278,7 @@ static bool ht16k33_keypad_scan(struct ht16k33_keypad *keypad)
> }
> }
> input_sync(keypad->dev);
> - memcpy(keypad->last_key_state, new_state, sizeof(new_state));
> + memcpy(keypad->last_key_state, new_state, sizeof(u16) * keypad->cols);
>
> return pressed;
> }
> @@ -353,6 +353,12 @@ static int ht16k33_keypad_probe(struct i2c_client *client,
> err = matrix_keypad_parse_of_params(&client->dev, &rows, &cols);
> if (err)
> return err;
> + if (rows > HT16K33_MATRIX_KEYPAD_MAX_ROWS ||
> + cols > HT16K33_MATRIX_KEYPAD_MAX_COLS) {
> + dev_err(&client->dev, "%u rows or %u cols out of range in DT\n",
> + rows, cols);
> + return -ERANGE;
> + }
>
> keypad->rows = rows;
> keypad->cols = cols;
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-03-29 8:07 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-28 10:11 [PATCH] auxdisplay: ht16k33: don't access uninitialized data Arnd Bergmann
2017-03-29 7:31 ` Dmitry Torokhov
2017-03-29 8:07 ` Robin van der Gracht
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.