* re: spi/fsl-espi: fix rx_buf in fsl_espi_cmd_trans()/fsl_espi_rw_trans()
@ 2014-05-28 9:01 Dan Carpenter
2014-05-28 15:06 ` Valentin Longchamp
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2014-05-28 9:01 UTC (permalink / raw
To: valentin.longchamp-SkAbAL50j+5BDgjK7y7TUQ
Cc: linux-spi-u79uwXL29TY76Z2rM5mHXA
Hello Valentin Longchamp,
I had a question about patch a2cb1be18254: "spi/fsl-espi: fix rx_buf in
fsl_espi_cmd_trans()/fsl_espi_rw_trans()" from May 16, 2014.
drivers/spi/spi-fsl-espi.c
396 espi_trans->n_tx = n_tx;
397 espi_trans->n_rx = trans_len;
398 espi_trans->len = trans_len + n_tx;
399 espi_trans->tx_buf = local_buf;
400 espi_trans->rx_buf = local_buf;
1) This is really weird that we share the same buffer for both sending
and receiving. My concern is that we've fixed the buffer overflow bug
by changing it to a memory corruption bug.
401 fsl_espi_do_trans(m, espi_trans);
402
403 memcpy(rx_buf + pos, espi_trans->rx_buf + n_tx, trans_len);
^^^^^^^^^^^^^^^^^^^^^^^^^
2) Why do we have the "+ n_tx" here? "n_tx + trans_len" was a buffer
before so that means this code is still reading beyond the end of the
array.
404
405 if (loop > 0)
406 espi_trans->actual_length += espi_trans->len - n_tx;
407 else
408 espi_trans->actual_length += espi_trans->len;
regards,
dan carpenter
--
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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: spi/fsl-espi: fix rx_buf in fsl_espi_cmd_trans()/fsl_espi_rw_trans()
2014-05-28 9:01 spi/fsl-espi: fix rx_buf in fsl_espi_cmd_trans()/fsl_espi_rw_trans() Dan Carpenter
@ 2014-05-28 15:06 ` Valentin Longchamp
0 siblings, 0 replies; 2+ messages in thread
From: Valentin Longchamp @ 2014-05-28 15:06 UTC (permalink / raw
To: Dan Carpenter; +Cc: linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Hello Dan,
On 05/28/2014 11:01 AM, Dan Carpenter wrote:
> I had a question about patch a2cb1be18254: "spi/fsl-espi: fix rx_buf in
> fsl_espi_cmd_trans()/fsl_espi_rw_trans()" from May 16, 2014.
>
> drivers/spi/spi-fsl-espi.c
> 396 espi_trans->n_tx = n_tx;
> 397 espi_trans->n_rx = trans_len;
> 398 espi_trans->len = trans_len + n_tx;
> 399 espi_trans->tx_buf = local_buf;
> 400 espi_trans->rx_buf = local_buf;
>
> 1) This is really weird that we share the same buffer for both sending
> and receiving. My concern is that we've fixed the buffer overflow bug
> by changing it to a memory corruption bug.
I don't think that there is a memory corruption bug. In the above case, a
read/write transfer is done (bits are shifted in/out at the same time of both
the MISO/MOSI lines), the first TX bytes has already been shifted out when it
gets overwritten by the then received RX byte, that's why the same buffer can be
used (even though I agree that it may be better to use separate ones as Mark
already said).
>
> 401 fsl_espi_do_trans(m, espi_trans);
> 402
> 403 memcpy(rx_buf + pos, espi_trans->rx_buf + n_tx, trans_len);
> ^^^^^^^^^^^^^^^^^^^^^^^^^
> 2) Why do we have the "+ n_tx" here? "n_tx + trans_len" was a buffer
> before so that means this code is still reading beyond the end of the
> array.
I don't think that "n_tx + trans_len" was a buffer before, it was also in the
same buffer (or I don't understand what you mean). And I think + "n_tx" is
necessary to only copy what was just read, which corresponds to the number of
bytes which have been shifted out (as explained above).
>
> 404
> 405 if (loop > 0)
> 406 espi_trans->actual_length += espi_trans->len - n_tx;
> 407 else
> 408 espi_trans->actual_length += espi_trans->len;
>
I just want to remind that I am quite new to this driver and maybe I have not
understood completely its behavior our forgotten some details so my explanations
could be wrong.
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2014-05-28 15:06 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-28 9:01 spi/fsl-espi: fix rx_buf in fsl_espi_cmd_trans()/fsl_espi_rw_trans() Dan Carpenter
2014-05-28 15:06 ` Valentin Longchamp
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.