[PATCH] serial: ifx6x60: avoid uninitialized variable use

[PATCH] serial: ifx6x60: avoid uninitialized variable use

[Arnd Bergmann]

gcc warns about a potential use of an uninitialized variable in this driver:

drivers/tty/serial/ifx6x60.c: In function 'ifx_spi_complete':
drivers/tty/serial/ifx6x60.c:713:6: warning: 'more' may be used uninitialized in this function [-Wmaybe-uninitialized]
   if (more || ifx_dev->spi_more || queue_length > 0 ||

Unlike a lot of other such warnings, this one is correct and describes
an actual problem in the handling of the "IFX_SPI_HEADER_F" result code.

This appears to be a result from a restructuring of the driver that
dates back to before it was merged in the kernel, so it's impossible
to know where it went wrong. I also don't know what that result code
means, so I have no idea if setting 'more' to zero is the correct
solution, but at least it makes the behavior reproducible rather than
depending on whatever happens to be on the kernel stack.

This patch initializes the 'more' variable to zero in each of the
three code paths that could result in undefined behavior before,
which is more explicit than initializing it at the start of the
function.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/tty/serial/ifx6x60.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/ifx6x60.c b/drivers/tty/serial/ifx6x60.c
index 88246f7e435a..2085a6cfa44b 100644
--- a/drivers/tty/serial/ifx6x60.c
+++ b/drivers/tty/serial/ifx6x60.c
@@ -395,8 +395,10 @@ static int ifx_spi_decode_spi_header(unsigned char *buffer, int *length,
 
 	if (h1 == 0 && h2 == 0) {
 		*received_cts = 0;
+		*more = 0;
 		return IFX_SPI_HEADER_0;
 	} else if (h1 == 0xffff && h2 == 0xffff) {
+		*more = 0;
 		/* spi_slave_cts remains as it was */
 		return IFX_SPI_HEADER_F;
 	}
@@ -688,6 +690,7 @@ static void ifx_spi_complete(void *ctx)
 			ifx_dev->rx_buffer + IFX_SPI_HEADER_OVERHEAD,
 			(size_t)actual_length);
 	} else {
+		more = 0;
 		dev_dbg(&ifx_dev->spi_dev->dev, "SPI transfer error %d",
 		       ifx_dev->spi_msg.status);
 	}
-- 
2.7.0

Re: [PATCH] serial: ifx6x60: avoid uninitialized variable use

[One Thousand Gnomes]

On Thu, 25 Feb 2016 21:47:57 +0100
Arnd Bergmann <arnd@arndb.de> wrote:

> gcc warns about a potential use of an uninitialized variable in this driver:
> 
> drivers/tty/serial/ifx6x60.c: In function 'ifx_spi_complete':
> drivers/tty/serial/ifx6x60.c:713:6: warning: 'more' may be used uninitialized in this function [-Wmaybe-uninitialized]
>    if (more || ifx_dev->spi_more || queue_length > 0 ||
> 
> Unlike a lot of other such warnings, this one is correct and describes
> an actual problem in the handling of the "IFX_SPI_HEADER_F" result code.
> 
> This appears to be a result from a restructuring of the driver that
> dates back to before it was merged in the kernel, so it's impossible
> to know where it went wrong. I also don't know what that result code
> means, so I have no idea if setting 'more' to zero is the correct
> solution, but at least it makes the behavior reproducible rather than
> depending on whatever happens to be on the kernel stack.

Would it not be far simpler just to set more = 0 at the top of
ifx_spi_complete ?

Re: [PATCH] serial: ifx6x60: avoid uninitialized variable use

[Arnd Bergmann]

On Friday 26 February 2016 00:06:51 One Thousand Gnomes wrote:
> On Thu, 25 Feb 2016 21:47:57 +0100
> Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > gcc warns about a potential use of an uninitialized variable in this driver:
> > 
> > drivers/tty/serial/ifx6x60.c: In function 'ifx_spi_complete':
> > drivers/tty/serial/ifx6x60.c:713:6: warning: 'more' may be used uninitialized in this function [-Wmaybe-uninitialized]
> >    if (more || ifx_dev->spi_more || queue_length > 0 ||
> > 
> > Unlike a lot of other such warnings, this one is correct and describes
> > an actual problem in the handling of the "IFX_SPI_HEADER_F" result code.
> > 
> > This appears to be a result from a restructuring of the driver that
> > dates back to before it was merged in the kernel, so it's impossible
> > to know where it went wrong. I also don't know what that result code
> > means, so I have no idea if setting 'more' to zero is the correct
> > solution, but at least it makes the behavior reproducible rather than
> > depending on whatever happens to be on the kernel stack.
> 
> Would it not be far simpler just to set more = 0 at the top of
> ifx_spi_complete ?
> 
> 

That would be simpler, but I generally don't like to do that, because it
makes it less obvious where the value is coming from.

In this case, it's still not obvious, as I was just guessing what the
original intention might have been.

	Arnd

Re: [PATCH] serial: ifx6x60: avoid uninitialized variable use

[Arnd Bergmann]

On Tuesday 24 November 2015 23:04:00 Arnd Bergmann wrote:
> gcc warns about a potential use of an uninitialized variable in this driver:
> 
> drivers/tty/serial/ifx6x60.c: In function 'ifx_spi_complete':
> drivers/tty/serial/ifx6x60.c:713:6: warning: 'more' may be used uninitialized in this function [-Wmaybe-uninitialized]
>    if (more || ifx_dev->spi_more || queue_length > 0 ||
> 
> Unlike a lot of other such warnings, this one is correct and describes
> an actual problem in the handling of the "IFX_SPI_HEADER_F" result code.
> 
> This appears to be a result from a restructuring of the driver that
> dates back to before it was merged in the kernel, so it's impossible
> to know where it went wrong. I also don't know what that result code
> means, so I have no idea if setting 'more' to zero is the correct
> solution, but at least it makes the behavior reproducible rather than
> depending on whatever happens to be on the kernel stack.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> Cc everyone who contributed non-cleanup patches to this driver,
> maybe someone has more insight into the operation of the driver than
> I have and can comment on whether this is the right fix or not.

Grmbl. Please don't apply this version for now.

The patch fixes one issue and made the warning go away in some
configurations, but my randconfig tests still show the same warning
in other configurations, as there is a second way that the 'more'
variable ends up being referenced without being initialized.

Let's wait for comments first, but I assume we will have to initialize
the 'more' variable as well, or possibly that function needs to
be rewritten.

	Arnd

[PATCH] serial: ifx6x60: avoid uninitialized variable use

[Arnd Bergmann]

gcc warns about a potential use of an uninitialized variable in this driver:

drivers/tty/serial/ifx6x60.c: In function 'ifx_spi_complete':
drivers/tty/serial/ifx6x60.c:713:6: warning: 'more' may be used uninitialized in this function [-Wmaybe-uninitialized]
   if (more || ifx_dev->spi_more || queue_length > 0 ||

Unlike a lot of other such warnings, this one is correct and describes
an actual problem in the handling of the "IFX_SPI_HEADER_F" result code.

This appears to be a result from a restructuring of the driver that
dates back to before it was merged in the kernel, so it's impossible
to know where it went wrong. I also don't know what that result code
means, so I have no idea if setting 'more' to zero is the correct
solution, but at least it makes the behavior reproducible rather than
depending on whatever happens to be on the kernel stack.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
Cc everyone who contributed non-cleanup patches to this driver,
maybe someone has more insight into the operation of the driver than
I have and can comment on whether this is the right fix or not.

diff --git a/drivers/tty/serial/ifx6x60.c b/drivers/tty/serial/ifx6x60.c
index 88246f7e435a..5d18442e42ba 100644
--- a/drivers/tty/serial/ifx6x60.c
+++ b/drivers/tty/serial/ifx6x60.c
@@ -397,6 +397,7 @@ static int ifx_spi_decode_spi_header(unsigned char *buffer, int *length,
 		*received_cts = 0;
 		return IFX_SPI_HEADER_0;
 	} else if (h1 == 0xffff && h2 == 0xffff) {
+		*received_cts = 0;
 		/* spi_slave_cts remains as it was */
 		return IFX_SPI_HEADER_F;
 	}