From: Purna Chandra Mandal <purna.mandal@microchip.com>
To: Mark Brown <broonie@kernel.org>
Cc: Joshua Henderson <joshua.henderson@microchip.com>,
<linux-kernel@vger.kernel.org>, <linux-spi@vger.kernel.org>
Subject: Re: [PATCH] spi: Fix incomplete handling of SPI_MASTER_MUST_RX/_MUST_TX
Date: Fri, 5 Feb 2016 10:30:24 +0530 [thread overview]
Message-ID: <56B42C68.4030909@microchip.com> (raw)
In-Reply-To: <20160201231733.GK4455@sirena.org.uk>
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.
next prev parent reply other threads:[~2016-02-05 5:01 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2016-02-08 16:15 ` Mark Brown
2016-03-01 12:17 ` Purna Chandra Mandal
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56B42C68.4030909@microchip.com \
--to=purna.mandal@microchip.com \
--cc=broonie@kernel.org \
--cc=joshua.henderson@microchip.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-spi@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).