linux-kernel - Re: [greybus-dev] [PATCH] greybus: uart: fix uninitialized flow control variable

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: <7146e1ca-1fc7-61ab-e798-7b017c7c974b@linaro.org>
Date:   Wed, 29 Apr 2020 15:00:44 -0500
From:   Alex Elder <elder@...aro.org>
To:     Arnd Bergmann <arnd@...db.de>, David Lin <dtwlin@...il.com>,
        Johan Hovold <johan@...nel.org>, Alex Elder <elder@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Axel Haslam <ahaslam@...libre.com>
Cc:     devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org,
        greybus-dev@...ts.linaro.org,
        Greg Kroah-Hartman <gregkh@...gle.com>,
        Johan Hovold <johan@...oldconsulting.com>
Subject: Re: [greybus-dev] [PATCH] greybus: uart: fix uninitialized flow
 control variable

On 4/29/20 2:00 PM, Arnd Bergmann wrote:
> gcc-10 points out an uninitialized variable use:

Wow, nice, checking individual uninitialized fields within
the structure.

The structure should really be zero-initialized anyway; it's
passed as a structure in a message elsewhere.  With your
change, all fields in the structure are written, but in
theory the structure could change and stack garbage could
be sent over the wire.

What do you think of doing this instead?  Or in addition?

        struct gb_tty_line_coding newline = { };

(Presumably that would also silence the warning.)

I endorse of your change, either way.

					-Alex

> drivers/staging/greybus/uart.c: In function 'gb_tty_set_termios':
> drivers/staging/greybus/uart.c:540:24: error: 'newline.flow_control' is used uninitialized in this function [-Werror=uninitialized]
>   540 |   newline.flow_control |= GB_SERIAL_AUTO_RTSCTS_EN;
> 
> Instead of using |= and &= on the uninitialized variable, use a
> direct assignment.
> 
> Fixes: e55c25206d5c ("greybus: uart: Handle CRTSCTS flag in termios")
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> ---
>  drivers/staging/greybus/uart.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c
> index 55c51143bb09..4ffb334cd5cd 100644
> --- a/drivers/staging/greybus/uart.c
> +++ b/drivers/staging/greybus/uart.c
> @@ -537,9 +537,9 @@ static void gb_tty_set_termios(struct tty_struct *tty,
>  	}
>  
>  	if (C_CRTSCTS(tty) && C_BAUD(tty) != B0)
> -		newline.flow_control |= GB_SERIAL_AUTO_RTSCTS_EN;
> +		newline.flow_control = GB_SERIAL_AUTO_RTSCTS_EN;
>  	else
> -		newline.flow_control &= ~GB_SERIAL_AUTO_RTSCTS_EN;
> +		newline.flow_control = 0;
>  
>  	if (memcmp(&gb_tty->line_coding, &newline, sizeof(newline))) {
>  		memcpy(&gb_tty->line_coding, &newline, sizeof(newline));
>