Skip to contentSkip to conversation

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

DISCUSSION Dan Carpenter 2 participants 2 messages May 28, 2014 SPI

lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#1 lore raw

Hello Dan,

Quoted 10 lines · Dan Carpenter

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;
Quoted 3 lines

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

Quoted 8 lines
   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).

Quoted 7 lines
   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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html