| Message ID | 1400251581-26617-1-git-send-email-valentin.longchamp@keymile.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a2cb1be18254fd1479d87f7860af7a8413508e16 |
| Headers | show |
On Fri, May 16, 2014 at 04:46:21PM +0200, Valentin Longchamp wrote: > By default for every espi transfer, the rx_buf is placed right after the > tx_buf. This can lead to a buffer overflow when the size of both the TX > and RX data cumulated is higher than the allocated 64K buffer for the > transfer (this is the case when sending for instance a read command and > reading 64K back, please see: > http://article.gmane.org/gmane.linux.drivers.mtd/53411 ) > > This gets fixed by always setting the RX buffer pointer at the begining > of the transfer buffer. This still doesn't seem safe - we're now going to be DMAing to and from the same buffer at the same time (and doubtless mapping it twice...). Would it not be safer to allocate separate rx and tx buffers? Indeed can we not use the core DMA mapping support to avoid the need to copy at all (it will construct scatterlists in PAGE_SIZE chunks)?
On 05/16/2014 09:07 PM, Mark Brown wrote: > On Fri, May 16, 2014 at 04:46:21PM +0200, Valentin Longchamp wrote: > >> By default for every espi transfer, the rx_buf is placed right after the >> tx_buf. This can lead to a buffer overflow when the size of both the TX >> and RX data cumulated is higher than the allocated 64K buffer for the >> transfer (this is the case when sending for instance a read command and >> reading 64K back, please see: >> http://article.gmane.org/gmane.linux.drivers.mtd/53411 ) >> >> This gets fixed by always setting the RX buffer pointer at the begining >> of the transfer buffer. > > This still doesn't seem safe - we're now going to be DMAing to and from > the same buffer at the same time (and doubtless mapping it twice...). > Would it not be safer to allocate separate rx and tx buffers? Indeed > can we not use the core DMA mapping support to avoid the need to copy at > all (it will construct scatterlists in PAGE_SIZE chunks)? > I agree that if the DMA does the transfers, to be completely safe we should have 2 separate buffers. But in the case of the spi-fsl-espi driver, I don't think that some DMA transfers to fill up the hardware buffers are involved. This is done by the CPU directly to the SPI_TF/SPI_RF registers since there are no real HW buffers accessible (maybe this is possible in the spi-fsl-cpm driver, where there are hints about DMA ?). Or are you talking about the initial memcpy call that fills up the local buffer in both rw_trans() and cmd_trans() ? And maybe your second point with the scatterlists is about replacing these memcpy calls ? Can you please precise because I have not fully understood your answer. Thanks Valentin -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 19, 2014 at 05:30:24PM +0200, Valentin Longchamp wrote: > Or are you talking about the initial memcpy call that fills up the local buffer > in both rw_trans() and cmd_trans() ? And maybe your second point with the > scatterlists is about replacing these memcpy calls ? We shouldn't need this copy at all and therefore there should be no need to make sure it is copying into an adequately sized buffer. The only thing I can think that the memcpy() is for is ensuring that the data is sent from a physically contiguous buffer but scatter/gather should handle that.
On 05/19/2014 06:22 PM, Mark Brown wrote: > On Mon, May 19, 2014 at 05:30:24PM +0200, Valentin Longchamp wrote: >> Or are you talking about the initial memcpy call that fills up the local buffer >> in both rw_trans() and cmd_trans() ? And maybe your second point with the >> scatterlists is about replacing these memcpy calls ? > > We shouldn't need this copy at all and therefore there should be no need > to make sure it is copying into an adequately sized buffer. The only > thing I can think that the memcpy() is for is ensuring that the data is > sent from a physically contiguous buffer but scatter/gather should > handle that. > OK I see your point. The problem is that I currently have no time to write/test a patch that replaces this unnecessary memcpy() to the buffer with scatter/gather, it will have to wait a bit. On the other hand, the patch I have sent fixes a buffer overflow with the current implementation so it could be good to take it if I don't send another patch with the above improvement. Valentin -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 16, 2014 at 04:46:21PM +0200, Valentin Longchamp wrote: > By default for every espi transfer, the rx_buf is placed right after the > tx_buf. This can lead to a buffer overflow when the size of both the TX > and RX data cumulated is higher than the allocated 64K buffer for the > transfer (this is the case when sending for instance a read command and > reading 64K back, please see: > http://article.gmane.org/gmane.linux.drivers.mtd/53411 ) Applied, thanks.
diff --git a/drivers/spi/spi-fsl-espi.c b/drivers/spi/spi-fsl-espi.c index e767f58..f17d6bd 100644 --- a/drivers/spi/spi-fsl-espi.c +++ b/drivers/spi/spi-fsl-espi.c @@ -348,7 +348,7 @@ static void fsl_espi_cmd_trans(struct spi_message *m, } espi_trans->tx_buf = local_buf; - espi_trans->rx_buf = local_buf + espi_trans->n_tx; + espi_trans->rx_buf = local_buf; fsl_espi_do_trans(m, espi_trans); espi_trans->actual_length = espi_trans->len; @@ -397,7 +397,7 @@ static void fsl_espi_rw_trans(struct spi_message *m, espi_trans->n_rx = trans_len; espi_trans->len = trans_len + n_tx; espi_trans->tx_buf = local_buf; - espi_trans->rx_buf = local_buf + n_tx; + espi_trans->rx_buf = local_buf; fsl_espi_do_trans(m, espi_trans); memcpy(rx_buf + pos, espi_trans->rx_buf + n_tx, trans_len);
By default for every espi transfer, the rx_buf is placed right after the tx_buf. This can lead to a buffer overflow when the size of both the TX and RX data cumulated is higher than the allocated 64K buffer for the transfer (this is the case when sending for instance a read command and reading 64K back, please see: http://article.gmane.org/gmane.linux.drivers.mtd/53411 ) This gets fixed by always setting the RX buffer pointer at the begining of the transfer buffer. Signed-off-by: Valentin Longchamp <valentin.longchamp@keymile.com> --- drivers/spi/spi-fsl-espi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)