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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170329073115.GF38261@dtor-ws>
Date:   Wed, 29 Mar 2017 00:31:15 -0700
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Robin van der Gracht <robin@...tonic.nl>,
        Miguel Ojeda Sandonis <miguel.ojeda.sandonis@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rob Herring <robh@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] auxdisplay: ht16k33: don't access uninitialized data

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@...db.de>

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@...il.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