Re: [PATCH] auxdisplay: ht16k33: don't access uninitialized data

[Robin van der Gracht]

On Tue, 28 Mar 2017 12:11:49 +0200
Arnd Bergmann <arnd@xxxxxxxx> 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@xxxxxxxx>

Reviewed-by: Robin van der Gracht <robin@xxxxxxxxxxx>

> ---
>  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;