* [PATCH] spi: Fix incomplete handling of SPI_MASTER_MUST_RX/_MUST_TX
@ 2016-02-01 22:39 Joshua Henderson
2016-02-01 23:17 ` Mark Brown
0 siblings, 1 reply; 5+ messages in thread
From: Joshua Henderson @ 2016-02-01 22:39 UTC (permalink / raw)
To: linux-kernel
Cc: Purna Chandra Mandal, Joshua Henderson, Mark Brown, linux-spi
From: Purna Chandra Mandal <purna.mandal@microchip.com>
There is a BUG in the way SPI_MASTER_MUST_RX/TX is implemented which can create
a kernel crash. To simplify design spi driver can specify *_MUST_RX during
registration. In these cases spi core do allocate & assign dummy RX buffer (of
right size) with the transfer if the transfer has NULL 'rx_buf'; at later point
the dummy buffer is free'd when the spi transfer (actually message containing
the transfer) is handled by respective master driver and no other spi messages
pending with the spi core.
This is where BUG is hiding!
(1) spi core assigns dummy_rx buffer to transfer.rx_buf member and
(2) passes it to lower layer for handling. and lower layer completed the
transfer/message in due time.
(3) spi core deletes the buffer if no other requests pending, but
'transfer.rx_buf' continues to hold *stale* dummy buffer pointer.
(4) If spi client driver (like mmc_spi) reuses the same transfer structure and
don't touch .rx_buf to NULL
mmc_spi doesn't reset the ptr unless data transfer direction changes in future
transaction(s). spi core will skip assigning new dummy buffer and underlying
master driver will treat .rx_buf as legitimate ptr. This will result into memory
corruption due to usage of free'd ptr.
Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
Signed-off-by: Joshua Henderson <joshua.henderson@microchip.com>
---
drivers/spi/spi.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 47eff80..deabd6f 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -819,6 +819,15 @@ static int __spi_unmap_msg(struct spi_master *master, struct spi_message *msg)
struct spi_transfer *xfer;
struct device *tx_dev, *rx_dev;
+ if (master->flags & (SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX)) {
+ list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+ if (xfer->rx_buf == master->dummy_rx)
+ xfer->rx_buf = NULL;
+ if (xfer->tx_buf == master->dummy_tx)
+ xfer->tx_buf = NULL;
+ }
+ }
+
if (!master->cur_msg_mapped || !master->can_dma)
return 0;
@@ -1264,12 +1273,11 @@ void spi_finalize_current_message(struct spi_master *master)
unsigned long flags;
int ret;
+ spi_unmap_msg(master, master->cur_msg);
spin_lock_irqsave(&master->queue_lock, flags);
mesg = master->cur_msg;
spin_unlock_irqrestore(&master->queue_lock, flags);
- spi_unmap_msg(master, mesg);
-
if (master->cur_msg_prepared && master->unprepare_message) {
ret = master->unprepare_message(master, mesg);
if (ret) {
--
1.7.9.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] spi: Fix incomplete handling of SPI_MASTER_MUST_RX/_MUST_TX
2016-02-01 22:39 [PATCH] spi: Fix incomplete handling of SPI_MASTER_MUST_RX/_MUST_TX Joshua Henderson
@ 2016-02-01 23:17 ` Mark Brown
2016-02-05 5:00 ` Purna Chandra Mandal
0 siblings, 1 reply; 5+ messages in thread
From: Mark Brown @ 2016-02-01 23:17 UTC (permalink / raw)
To: Joshua Henderson; +Cc: linux-kernel, Purna Chandra Mandal, linux-spi
[-- Attachment #1: Type: text/plain, Size: 1266 bytes --]
On Mon, Feb 01, 2016 at 03:39:23PM -0700, Joshua Henderson wrote:
> From: Purna Chandra Mandal <purna.mandal@microchip.com>
> There is a BUG in the way SPI_MASTER_MUST_RX/TX is implemented which can create
Bug is a WORD like any other WORD...
> (1) spi core assigns dummy_rx buffer to transfer.rx_buf member and
> (2) passes it to lower layer for handling. and lower layer completed the
> transfer/message in due time.
> (3) spi core deletes the buffer if no other requests pending, but
> 'transfer.rx_buf' continues to hold *stale* dummy buffer pointer.
> (4) If spi client driver (like mmc_spi) reuses the same transfer structure and
> don't touch .rx_buf to NULL
> mmc_spi doesn't reset the ptr unless data transfer direction changes in future
> transaction(s). spi core will skip assigning new dummy buffer and underlying
> master driver will treat .rx_buf as legitimate ptr. This will result into memory
> corruption due to usage of free'd ptr.
It's not clear to me that this is the best fix, it's causing problems to
free the transfer but we could also fix that by just not freeing the
dummy data once we realize we need it unless the adaptor is freed. That
should also be more efficient since it saves us having to allocate and
free things.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] spi: Fix incomplete handling of SPI_MASTER_MUST_RX/_MUST_TX
2016-02-01 23:17 ` Mark Brown
@ 2016-02-05 5:00 ` Purna Chandra Mandal
2016-02-08 16:15 ` Mark Brown
0 siblings, 1 reply; 5+ messages in thread
From: Purna Chandra Mandal @ 2016-02-05 5:00 UTC (permalink / raw)
To: Mark Brown; +Cc: Joshua Henderson, linux-kernel, linux-spi
Thanks Mark.
On 02/02/2016 04:47 AM, Mark Brown wrote:
> On Mon, Feb 01, 2016 at 03:39:23PM -0700, Joshua Henderson wrote:
>> From: Purna Chandra Mandal <purna.mandal@microchip.com>
>> There is a BUG in the way SPI_MASTER_MUST_RX/TX is implemented which can create
> Bug is a WORD like any other WORD...
ack.
>> (1) spi core assigns dummy_rx buffer to transfer.rx_buf member and
>> (2) passes it to lower layer for handling. and lower layer completed the
>> transfer/message in due time.
>> (3) spi core deletes the buffer if no other requests pending, but
>> 'transfer.rx_buf' continues to hold *stale* dummy buffer pointer.
>> (4) If spi client driver (like mmc_spi) reuses the same transfer structure and
>> don't touch .rx_buf to NULL
>> mmc_spi doesn't reset the ptr unless data transfer direction changes in future
>> transaction(s). spi core will skip assigning new dummy buffer and underlying
>> master driver will treat .rx_buf as legitimate ptr. This will result into memory
>> corruption due to usage of free'd ptr.
> It's not clear to me that this is the best fix, it's causing problems to
> free the transfer but we could also fix that by just not freeing the
> dummy data once we realize we need it unless the adaptor is freed. That
> should also be more efficient since it saves us having to allocate and
> free things.
Idea is good, but not sufficient.
Dummy buffers are _reallocated_ to accommodate larger size of transfer. In this if
[originally NULL] .rx_buf/.tx_buf is not reset back to NULL after the transfer
is over spi-core will find those .rx/tx_buf non-NULL in next _transfer() call and
will pass the stale rx/tx_buf to spi controller driver which will definitely
corrupt the memory and crash the system.
Above all the whole design depends on trust that core will not play with any data-structure
which will break object-oriented/layered approach. So better to undo the modification
(when needed to facilitate some functionality) unless core wants those information to be passed
back to caller/client for reporting success or error or else.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] spi: Fix incomplete handling of SPI_MASTER_MUST_RX/_MUST_TX
2016-02-05 5:00 ` Purna Chandra Mandal
@ 2016-02-08 16:15 ` Mark Brown
2016-03-01 12:17 ` Purna Chandra Mandal
0 siblings, 1 reply; 5+ messages in thread
From: Mark Brown @ 2016-02-08 16:15 UTC (permalink / raw)
To: Purna Chandra Mandal; +Cc: Joshua Henderson, linux-kernel, linux-spi
[-- Attachment #1: Type: text/plain, Size: 1284 bytes --]
On Fri, Feb 05, 2016 at 10:30:24AM +0530, Purna Chandra Mandal wrote:
Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns. Doing this makes your messages much
easier to read and reply to.
> Idea is good, but not sufficient.
> Dummy buffers are _reallocated_ to accommodate larger size of transfer. In this if
> [originally NULL] .rx_buf/.tx_buf is not reset back to NULL after the transfer
> is over spi-core will find those .rx/tx_buf non-NULL in next _transfer() call and
> will pass the stale rx/tx_buf to spi controller driver which will definitely
> corrupt the memory and crash the system.
This needs to be clear to readers; a fairly obvious way of dealing with
this would be to rellocate down to a page rather than freeing.
> Above all the whole design depends on trust that core will not play with any data-structure
> which will break object-oriented/layered approach. So better to undo the modification
> (when needed to facilitate some functionality) unless core wants those information to be passed
> back to caller/client for reporting success or error or else.
That's really not the case, we already make a range of other
modifications to complete partially filled transfers in order to
simplify driver code.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] spi: Fix incomplete handling of SPI_MASTER_MUST_RX/_MUST_TX
2016-02-08 16:15 ` Mark Brown
@ 2016-03-01 12:17 ` Purna Chandra Mandal
0 siblings, 0 replies; 5+ messages in thread
From: Purna Chandra Mandal @ 2016-03-01 12:17 UTC (permalink / raw)
To: Mark Brown; +Cc: Joshua Henderson, linux-kernel, linux-spi
Mark,
On 02/08/2016 09:45 PM, Mark Brown wrote:
> On Fri, Feb 05, 2016 at 10:30:24AM +0530, Purna Chandra Mandal wrote:
>
> Please fix your mail client to word wrap within paragraphs at something
> substantially less than 80 columns. Doing this makes your messages much
> easier to read and reply to.
>
>> Idea is good, but not sufficient.
>> Dummy buffers are _reallocated_ to accommodate larger size of transfer. In this if
>> [originally NULL] .rx_buf/.tx_buf is not reset back to NULL after the transfer
>> is over spi-core will find those .rx/tx_buf non-NULL in next _transfer() call and
>> will pass the stale rx/tx_buf to spi controller driver which will definitely
>> corrupt the memory and crash the system.
> This needs to be clear to readers; a fairly obvious way of dealing with
> this would be to rellocate down to a page rather than freeing.
Yea. But current krealloc() implementation allocates new memory if new size
is more than the allocated space, (and frees the old). If we allocate
PAGE_SIZE of dummy buffer (at first call irrespective of required size),
re-use it and don't allow transfer size to grow more than PAGE_SIZE we
will be fine provided all SPI clients agree to the size restriction.
The moment we'll try to re-allocate new buffer we will reach the same
point we wanted to avoid here.
>> Above all the whole design depends on trust that core will not play with any data-structure
>> which will break object-oriented/layered approach. So better to undo the modification
>> (when needed to facilitate some functionality) unless core wants those information to be passed
>> back to caller/client for reporting success or error or else.
> That's really not the case, we already make a range of other
> modifications to complete partially filled transfers in order to
> simplify driver code.
Purna
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-03-01 12:19 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-01 22:39 [PATCH] spi: Fix incomplete handling of SPI_MASTER_MUST_RX/_MUST_TX Joshua Henderson
2016-02-01 23:17 ` Mark Brown
2016-02-05 5:00 ` Purna Chandra Mandal
2016-02-08 16:15 ` Mark Brown
2016-03-01 12:17 ` Purna Chandra Mandal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).