linux-kernel - Bug in spi: imx: Add support for SPI Slave mode

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: <1551217306.3075.188.camel@impinj.com>
Date:   Tue, 26 Feb 2019 21:41:46 +0000
From:   Trent Piepho <tpiepho@...inj.com>
To:     "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "jiada_wang@...tor.com" <jiada_wang@...tor.com>,
        "fabio.estevam@....com" <fabio.estevam@....com>,
        "broonie@...nel.org" <broonie@...nel.org>
CC:     "linux-spi@...r.kernel.org" <linux-spi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "stefan@...er.ch" <stefan@...er.ch>
Subject: Bug in spi: imx: Add support for SPI Slave mode

On Tue, 2017-09-05 at 14:12 +0900, Jiada Wang wrote:
> Previously i.MX SPI controller only works in Master mode.
> This patch adds support to i.MX51, i.MX53 and i.MX6 ECSPI
> controller to work also in Slave mode.

Recently DMA has been enabled for imx6/7 with SPI.  This results in
memory corruption in some instances I've traced the problem to this
patch.

>  static int spi_imx_transfer(struct spi_device *spi,
>  				struct spi_transfer *transfer)
>  {
>  	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
>  
> +	/* flush rxfifo before transfer */
> +	while (spi_imx->devtype_data->rx_available(spi_imx))
> +		spi_imx->rx(spi_imx);
> +
> +	if (spi_imx->slave_mode)
> +		return spi_imx_pio_transfer_slave(spi, transfer);
> +
>  	if (spi_imx->usedma)
>  		return spi_imx_dma_transfer(spi_imx, transfer);
>  	else

This is in the main xfer function that is used for both master mode and
slave mode.  Normally RX data is received after the xfer is started, as
part of the spi_imx_pio/dma_transfer() process.  But this patch has
added a "flush rxfifo" command before this.  Why?

If it's necessary to empty the fifo before an xfer, then how did this
driver ever work before this change?  I see no change anywhere else
that would make this a new requirement.

If the rx fifo is not empty, and this code actually does rx something
from the fifo, then how can it possibly work to place it into the
xfer's RX buffer? How do you know the buffer is large enough (it's not!
that's my DMA bug!)?  Won't it offset the actual rx data that comes
after it in the xfer's buffer?

In my test, switching from DMA to PIO, which happens if the 1st xfer is
 large enough to pass a >fifo/2 size test, and uses DMA, and the 2nd
xfer is smaller, and will use PIO, results in the PIO xfer trying to
empty the rx buffer in this code above.  If there is not enough space
in the xfer's rx buffer, and there is no reason for there to be any
space at all, it clobbers memory.

I suspect the author of this wasn't aware that spi_imx->rx() will write
the data received into the current xfer's rx buffer rather than throw
it away, and never tested this actually getting used for a transfer
where the rx data mattered.

Still, I'd like to know why the flush rx thing is even here.  Nothing
in the commit message or any discussion I could find addressed this.