[PATCH] spi/fsl-espi: fix rx_buf in fsl_espi_cmd_trans()/fsl_espi_rw_trans()

[PATCH] spi/fsl-espi: fix rx_buf in fsl_espi_cmd_trans()/fsl_espi_rw_trans()

[Valentin Longchamp]

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-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org>

---

 drivers/spi/spi-fsl-espi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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);
-- 
1.8.0.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi/fsl-espi: fix rx_buf in fsl_espi_cmd_trans()/fsl_espi_rw_trans()

[Mark Brown]

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

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)?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi/fsl-espi: fix rx_buf in fsl_espi_cmd_trans()/fsl_espi_rw_trans()

[Valentin Longchamp]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi/fsl-espi: fix rx_buf in fsl_espi_cmd_trans()/fsl_espi_rw_trans()

[Mark Brown]

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

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi/fsl-espi: fix rx_buf in fsl_espi_cmd_trans()/fsl_espi_rw_trans()

[Valentin Longchamp]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi/fsl-espi: fix rx_buf in fsl_espi_cmd_trans()/fsl_espi_rw_trans()

[Mark Brown]

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

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]