[PATCH] spi: rspi: avoid uninitialized variable access

[PATCH] spi: rspi: avoid uninitialized variable access

[Arnd Bergmann]

The newly introduced rspi_pio_transfer_in_or_our() function must
take either a valid 'rx' or 'tx' pointer, and has undefined behavior
if both are NULL, as found by 'gcc -Wmaybe-unintialized':

drivers/spi/spi-rspi.c: In function 'rspi_pio_transfer_in_or_our':
drivers/spi/spi-rspi.c:558:5: error: 'len' may be used uninitialized in this function [-Werror=maybe-uninitialized]

The analysis of the function is correct in principle, but the code
is currently safe because both callers always pass exactly one
of the two pointers.

Looking closer at this function shows that having a combined
method for rx and tx here actually increases the complexity
and the size of the file. This simplifies it again by keeping
the two separate, which then ends up avoiding that warning.

Fixes: 3be09bec42a8 ("spi: rspi: supports 32bytes buffer for DUAL and QUAD")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/spi/spi-rspi.c | 94 ++++++++++++++++++++++++--------------------------
 1 file changed, 45 insertions(+), 49 deletions(-)

diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
index 3bab75ab1b25..9daf50031737 100644
--- a/drivers/spi/spi-rspi.c
+++ b/drivers/spi/spi-rspi.c
@@ -515,51 +515,6 @@ static int rspi_pio_transfer(struct rspi_data *rspi, const u8 *tx, u8 *rx,
 	return 0;
 }
 
-static int rspi_pio_transfer_in_or_our(struct rspi_data *rspi, const u8 *tx,
-				       u8 *rx, unsigned int n)
-{
-	unsigned int i, len;
-	int ret;
-
-	while (n > 0) {
-		if (tx) {
-			len = qspi_set_send_trigger(rspi, n);
-			if (len == QSPI_BUFFER_SIZE) {
-				ret = rspi_wait_for_tx_empty(rspi);
-				if (ret < 0) {
-					dev_err(&rspi->master->dev, "transmit timeout\n");
-					return ret;
-				}
-				for (i = 0; i < len; i++)
-					rspi_write_data(rspi, *tx++);
-			} else {
-				ret = rspi_pio_transfer(rspi, tx, NULL, n);
-				if (ret < 0)
-					return ret;
-			}
-		}
-		if (rx) {
-			len = qspi_set_receive_trigger(rspi, n);
-			if (len == QSPI_BUFFER_SIZE) {
-				ret = rspi_wait_for_rx_full(rspi);
-				if (ret < 0) {
-					dev_err(&rspi->master->dev, "receive timeout\n");
-					return ret;
-				}
-				for (i = 0; i < len; i++)
-					*rx++ = rspi_read_data(rspi);
-			} else {
-				ret = rspi_pio_transfer(rspi, NULL, rx, n);
-				if (ret < 0)
-					return ret;
-				*rx++ = ret;
-			}
-		}
-		n -= len;
-	}
-	return 0;
-}
-
 static void rspi_dma_complete(void *arg)
 {
 	struct rspi_data *rspi = arg;
@@ -831,6 +786,9 @@ static int qspi_transfer_out_in(struct rspi_data *rspi,
 
 static int qspi_transfer_out(struct rspi_data *rspi, struct spi_transfer *xfer)
 {
+	const u8 *tx = xfer->tx_buf;
+	unsigned int n = xfer->len;
+	unsigned int i, len;
 	int ret;
 
 	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) {
@@ -839,9 +797,23 @@ static int qspi_transfer_out(struct rspi_data *rspi, struct spi_transfer *xfer)
 			return ret;
 	}
 
-	ret = rspi_pio_transfer_in_or_our(rspi, xfer->tx_buf, NULL, xfer->len);
-	if (ret < 0)
-		return ret;
+	while (n > 0) {
+		len = qspi_set_send_trigger(rspi, n);
+		if (len == QSPI_BUFFER_SIZE) {
+			ret = rspi_wait_for_tx_empty(rspi);
+			if (ret < 0) {
+				dev_err(&rspi->master->dev, "transmit timeout\n");
+				return ret;
+			}
+			for (i = 0; i < len; i++)
+				rspi_write_data(rspi, *tx++);
+		} else {
+			ret = rspi_pio_transfer(rspi, tx, NULL, n);
+			if (ret < 0)
+				return ret;
+		}
+		n -= len;
+	}
 
 	/* Wait for the last transmission */
 	rspi_wait_for_tx_empty(rspi);
@@ -851,13 +823,37 @@ static int qspi_transfer_out(struct rspi_data *rspi, struct spi_transfer *xfer)
 
 static int qspi_transfer_in(struct rspi_data *rspi, struct spi_transfer *xfer)
 {
+	u8 *rx = xfer->rx_buf;
+	unsigned int n = xfer->len;
+	unsigned int i, len;
+	int ret;
+
 	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) {
 		int ret = rspi_dma_transfer(rspi, NULL, &xfer->rx_sg);
 		if (ret != -EAGAIN)
 			return ret;
 	}
 
-	return rspi_pio_transfer_in_or_our(rspi, NULL, xfer->rx_buf, xfer->len);
+	while (n > 0) {
+		len = qspi_set_receive_trigger(rspi, n);
+		if (len == QSPI_BUFFER_SIZE) {
+			ret = rspi_wait_for_rx_full(rspi);
+			if (ret < 0) {
+				dev_err(&rspi->master->dev, "receive timeout\n");
+				return ret;
+			}
+			for (i = 0; i < len; i++)
+				*rx++ = rspi_read_data(rspi);
+		} else {
+			ret = rspi_pio_transfer(rspi, NULL, rx, n);
+			if (ret < 0)
+				return ret;
+			*rx++ = ret;
+		}
+		n -= len;
+	}
+
+	return 0;
 }
 
 static int qspi_transfer_one(struct spi_master *master, struct spi_device *spi,
-- 
2.9.0

RE: [PATCH] spi: rspi: avoid uninitialized variable access

[Chris Brandt]

Since I was CC-ed, I'll add in my opinion:


While Geert already pointed out the spelling mistake (_in_or_our >> _in_or_out), since that function is only just for qspi versions, a better function name should have been "qspi_pio_transfer_in_or_out"



However....

On 11/8/2016, Arnd Bergmann wrote:
> This simplifies it again by keeping the two separate, which then ends up
> avoiding that warning.

I agree with Arnd's method of NOT adding a new "rspi_pio_transfer_in_or_our" function and instead just doing it in the existing qspi_transfer_ functions.



Side note: The RSPI in the RZ/A1 devices also have FIFOs which can be used to reduce the number of interrupts in pio transfers, so maybe someday I'll make a similar change for non-qspi devices as well.


Chris

Re: [PATCH] spi: rspi: avoid uninitialized variable access

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 729 bytes --]

On Tue, Nov 08, 2016 at 05:20:50PM +0000, Chris Brandt wrote:
> Since I was CC-ed, I'll add in my opinion:

> While Geert already pointed out the spelling mistake (_in_or_our >> _in_or_out), since that function is only just for qspi versions, a better function name should have been "qspi_pio_transfer_in_or_out"

Please don't top post, reply in line with needed context.  This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH] spi: rspi: avoid uninitialized variable access

[Geert Uytterhoeven]

On Tue, Nov 8, 2016 at 6:20 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> However....
>
> On 11/8/2016, Arnd Bergmann wrote:
>> This simplifies it again by keeping the two separate, which then ends up
>> avoiding that warning.
>
> I agree with Arnd's method of NOT adding a new "rspi_pio_transfer_in_or_our"
> function and instead just doing it in the existing qspi_transfer_ functions.
>
> Side note: The RSPI in the RZ/A1 devices also have FIFOs which can be used
> to reduce the number of interrupts in pio transfers, so maybe someday I'll
> make a similar change for non-qspi devices as well.

At which point we probably want to extract the functionality into two separate
functions again, instead of inlining into qspi_transfer_{in,out}()...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Applied "spi: rspi: avoid uninitialized variable access" to the spi tree

[Mark Brown]

The patch

   spi: rspi: avoid uninitialized variable access

has been applied to the spi tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From db30083813b559e98e10ae26bd09d3dc69be7fb7 Mon Sep 17 00:00:00 2001
From: Arnd Bergmann <arnd@arndb.de>
Date: Tue, 8 Nov 2016 14:46:12 +0100
Subject: [PATCH] spi: rspi: avoid uninitialized variable access

The newly introduced rspi_pio_transfer_in_or_our() function must
take either a valid 'rx' or 'tx' pointer, and has undefined behavior
if both are NULL, as found by 'gcc -Wmaybe-unintialized':

drivers/spi/spi-rspi.c: In function 'rspi_pio_transfer_in_or_our':
drivers/spi/spi-rspi.c:558:5: error: 'len' may be used uninitialized in this function [-Werror=maybe-uninitialized]

The analysis of the function is correct in principle, but the code
is currently safe because both callers always pass exactly one
of the two pointers.

Looking closer at this function shows that having a combined
method for rx and tx here actually increases the complexity
and the size of the file. This simplifies it again by keeping
the two separate, which then ends up avoiding that warning.

Fixes: 3be09bec42a8 ("spi: rspi: supports 32bytes buffer for DUAL and QUAD")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-rspi.c | 94 ++++++++++++++++++++++++--------------------------
 1 file changed, 45 insertions(+), 49 deletions(-)

diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
index 3bab75ab1b25..9daf50031737 100644
--- a/drivers/spi/spi-rspi.c
+++ b/drivers/spi/spi-rspi.c
@@ -515,51 +515,6 @@ static int rspi_pio_transfer(struct rspi_data *rspi, const u8 *tx, u8 *rx,
 	return 0;
 }
 
-static int rspi_pio_transfer_in_or_our(struct rspi_data *rspi, const u8 *tx,
-				       u8 *rx, unsigned int n)
-{
-	unsigned int i, len;
-	int ret;
-
-	while (n > 0) {
-		if (tx) {
-			len = qspi_set_send_trigger(rspi, n);
-			if (len == QSPI_BUFFER_SIZE) {
-				ret = rspi_wait_for_tx_empty(rspi);
-				if (ret < 0) {
-					dev_err(&rspi->master->dev, "transmit timeout\n");
-					return ret;
-				}
-				for (i = 0; i < len; i++)
-					rspi_write_data(rspi, *tx++);
-			} else {
-				ret = rspi_pio_transfer(rspi, tx, NULL, n);
-				if (ret < 0)
-					return ret;
-			}
-		}
-		if (rx) {
-			len = qspi_set_receive_trigger(rspi, n);
-			if (len == QSPI_BUFFER_SIZE) {
-				ret = rspi_wait_for_rx_full(rspi);
-				if (ret < 0) {
-					dev_err(&rspi->master->dev, "receive timeout\n");
-					return ret;
-				}
-				for (i = 0; i < len; i++)
-					*rx++ = rspi_read_data(rspi);
-			} else {
-				ret = rspi_pio_transfer(rspi, NULL, rx, n);
-				if (ret < 0)
-					return ret;
-				*rx++ = ret;
-			}
-		}
-		n -= len;
-	}
-	return 0;
-}
-
 static void rspi_dma_complete(void *arg)
 {
 	struct rspi_data *rspi = arg;
@@ -831,6 +786,9 @@ static int qspi_transfer_out_in(struct rspi_data *rspi,
 
 static int qspi_transfer_out(struct rspi_data *rspi, struct spi_transfer *xfer)
 {
+	const u8 *tx = xfer->tx_buf;
+	unsigned int n = xfer->len;
+	unsigned int i, len;
 	int ret;
 
 	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) {
@@ -839,9 +797,23 @@ static int qspi_transfer_out(struct rspi_data *rspi, struct spi_transfer *xfer)
 			return ret;
 	}
 
-	ret = rspi_pio_transfer_in_or_our(rspi, xfer->tx_buf, NULL, xfer->len);
-	if (ret < 0)
-		return ret;
+	while (n > 0) {
+		len = qspi_set_send_trigger(rspi, n);
+		if (len == QSPI_BUFFER_SIZE) {
+			ret = rspi_wait_for_tx_empty(rspi);
+			if (ret < 0) {
+				dev_err(&rspi->master->dev, "transmit timeout\n");
+				return ret;
+			}
+			for (i = 0; i < len; i++)
+				rspi_write_data(rspi, *tx++);
+		} else {
+			ret = rspi_pio_transfer(rspi, tx, NULL, n);
+			if (ret < 0)
+				return ret;
+		}
+		n -= len;
+	}
 
 	/* Wait for the last transmission */
 	rspi_wait_for_tx_empty(rspi);
@@ -851,13 +823,37 @@ static int qspi_transfer_out(struct rspi_data *rspi, struct spi_transfer *xfer)
 
 static int qspi_transfer_in(struct rspi_data *rspi, struct spi_transfer *xfer)
 {
+	u8 *rx = xfer->rx_buf;
+	unsigned int n = xfer->len;
+	unsigned int i, len;
+	int ret;
+
 	if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) {
 		int ret = rspi_dma_transfer(rspi, NULL, &xfer->rx_sg);
 		if (ret != -EAGAIN)
 			return ret;
 	}
 
-	return rspi_pio_transfer_in_or_our(rspi, NULL, xfer->rx_buf, xfer->len);
+	while (n > 0) {
+		len = qspi_set_receive_trigger(rspi, n);
+		if (len == QSPI_BUFFER_SIZE) {
+			ret = rspi_wait_for_rx_full(rspi);
+			if (ret < 0) {
+				dev_err(&rspi->master->dev, "receive timeout\n");
+				return ret;
+			}
+			for (i = 0; i < len; i++)
+				*rx++ = rspi_read_data(rspi);
+		} else {
+			ret = rspi_pio_transfer(rspi, NULL, rx, n);
+			if (ret < 0)
+				return ret;
+			*rx++ = ret;
+		}
+		n -= len;
+	}
+
+	return 0;
 }
 
 static int qspi_transfer_one(struct spi_master *master, struct spi_device *spi,
-- 
2.10.2

Re: [PATCH] spi: rspi: avoid uninitialized variable access

[Hiep Cao Minh]

Hi Arnd,

Thanks for your fixed patch.

On 11/08/2016 10:46 PM, Arnd Bergmann wrote:
> The newly introduced rspi_pio_transfer_in_or_our() function must
> take either a valid 'rx' or 'tx' pointer, and has undefined behavior
> if both are NULL, as found by 'gcc -Wmaybe-unintialized':
>
> drivers/spi/spi-rspi.c: In function 'rspi_pio_transfer_in_or_our':
> drivers/spi/spi-rspi.c:558:5: error: 'len' may be used uninitialized in this function [-Werror=maybe-uninitialized]
Could you tell me what kind of GCC are you using?
I'd like to reproduce it on my environment, too.
I am using the Linaro's gcc of 
"gcc-linaro-arm-linux-gnueabihf-4.8-2014.04_linux".
But there is no error message like this on my environment.


Best regards,
Jinzai Solution Inc,
Hiep.

Re: [PATCH] spi: rspi: avoid uninitialized variable access

[Arnd Bergmann]

On Thursday, November 10, 2016 6:25:56 PM CET Hiep Cao Minh wrote:
> Hi Arnd,
> 
> Thanks for your fixed patch.
> 
> On 11/08/2016 10:46 PM, Arnd Bergmann wrote:
> > The newly introduced rspi_pio_transfer_in_or_our() function must
> > take either a valid 'rx' or 'tx' pointer, and has undefined behavior
> > if both are NULL, as found by 'gcc -Wmaybe-unintialized':
> >
> > drivers/spi/spi-rspi.c: In function 'rspi_pio_transfer_in_or_our':
> > drivers/spi/spi-rspi.c:558:5: error: 'len' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> Could you tell me what kind of GCC are you using?
> I'd like to reproduce it on my environment, too.
> I am using the Linaro's gcc of 
> "gcc-linaro-arm-linux-gnueabihf-4.8-2014.04_linux".
> But there is no error message like this on my environment.

The warning is currently disabled in mainline Linux, but I'm trying to
address this and hope to still get a revert of 6e8d666e9253 ("Disable
"maybe-uninitialized" warning globally") into v4.9.

You can build with "make EXTRA_CFLAGS=-Wmaybe-uninitialized" in
the meantime.

	Arnd

Re: [PATCH] spi: rspi: avoid uninitialized variable access

[Hiep Cao Minh]

Hi Arnd,

On 11/10/2016 07:51 PM, Arnd Bergmann wrote:
> On Thursday, November 10, 2016 6:25:56 PM CET Hiep Cao Minh wrote:
>> Hi Arnd,
>>
>> Thanks for your fixed patch.
>>
>> On 11/08/2016 10:46 PM, Arnd Bergmann wrote:
>>> The newly introduced rspi_pio_transfer_in_or_our() function must
>>> take either a valid 'rx' or 'tx' pointer, and has undefined behavior
>>> if both are NULL, as found by 'gcc -Wmaybe-unintialized':
>>>
>>> drivers/spi/spi-rspi.c: In function 'rspi_pio_transfer_in_or_our':
>>> drivers/spi/spi-rspi.c:558:5: error: 'len' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>> Could you tell me what kind of GCC are you using?
>> I'd like to reproduce it on my environment, too.
>> I am using the Linaro's gcc of
>> "gcc-linaro-arm-linux-gnueabihf-4.8-2014.04_linux".
>> But there is no error message like this on my environment.
> The warning is currently disabled in mainline Linux, but I'm trying to
> address this and hope to still get a revert of 6e8d666e9253 ("Disable
> "maybe-uninitialized" warning globally") into v4.9.
>
> You can build with "make EXTRA_CFLAGS=-Wmaybe-uninitialized" in
> the meantime.
Thanks,
I used your command and found the warning.

Best regards,
Hiep
Jinzai Solution Inc,