linux-kernel - RE: [PATCH] i2c: i2c-tegra: fix possible race condition after tx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 3 Jun 2011 15:01:49 -0700
From:	Stephen Warren <swarren@...dia.com>
To:	Vincent Palatin <vpalatin@...omium.org>,
	Jean Delvare <khali@...ux-fr.org>,
	Ben Dooks <ben-linux@...ff.org>,
	"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>
CC:	Olof Johansson <olofj@...omium.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Colin Cross <ccross@...roid.com>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>
Subject: RE: [PATCH] i2c: i2c-tegra: fix possible race condition after tx

Vincent Palatin wrote at Thursday, June 02, 2011 7:32 AM:
> Anyone has a comment on that patch ?
> The I2C driver has been reworked but this issue seems to still exist

Tested-by: Stephen Warren <swarren@...dia.com>

(using code based on 3.0-rc1, on Harmony, ran "speaker-test -c 2", and
then adjusted the volume a lot using alsamixer, thus causing quite a few
I2C transactions)

One question inline below though.

> On Tue, Apr 19, 2011 at 18:14, Vincent Palatin <vpalatin@...omium.org> wrote:
> > In tegra_i2c_fill_tx_fifo, once we have finished pushing all the bytes
> > to the I2C hardware controller, the interrupt might happen before we
> > have updated i2c_dev->msg_buf_remaining at the end of the function.
> > Then, in tegra_i2c_isr, we will call again tegra_i2c_fill_tx_fifo
> > triggering weird behaviour.
> > Of course, this is unlikely since the I2C bus is slow and thus the ISR
> > is called several hundreds of microseconds after the last register
> > write.
> >
> > Signed-off-by: Vincent Palatin <vpalatin@...omium.org>
> > ---
> >  drivers/i2c/busses/i2c-tegra.c |   54 ++++++++++++++++++++++-----------------
> >  1 files changed, 30 insertions(+), 24 deletions(-)
...
> > @@ -213,38 +213,41 @@ static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev)
> >        u32 val;
> >        int rx_fifo_avail;
> >        u8 *buf = i2c_dev->msg_buf;
> > -       size_t buf_remaining = i2c_dev->msg_buf_remaining;

The old code read msg_buf_remaining once up front and did everything
based on that.

> >        int words_to_transfer;
> > +       int bytes_to_transfer;
> >
> >        val = i2c_readl(i2c_dev, I2C_FIFO_STATUS);
> >        rx_fifo_avail = (val & I2C_FIFO_STATUS_RX_MASK) >>
> >                I2C_FIFO_STATUS_RX_SHIFT;
> >
> >        /* Rounds down to not include partial word at the end of buf */
> > -       words_to_transfer = buf_remaining / BYTES_PER_FIFO_WORD;
> > +       words_to_transfer = atomic_read(&i2c_dev->msg_buf_remaining) /
> > +               BYTES_PER_FIFO_WORD;

Whereas the new code reads msg_buf_remaining once here...

> >        if (words_to_transfer > rx_fifo_avail)
> >                words_to_transfer = rx_fifo_avail;
> >
> > +       atomic_sub(words_to_transfer * BYTES_PER_FIFO_WORD,
> > +               &i2c_dev->msg_buf_remaining);
> >        i2c_readsl(i2c_dev, buf, I2C_RX_FIFO, words_to_transfer);
> >
> >        buf += words_to_transfer * BYTES_PER_FIFO_WORD;
> > -       buf_remaining -= words_to_transfer * BYTES_PER_FIFO_WORD;
> >        rx_fifo_avail -= words_to_transfer;
> >
> >        /*
> >         * If there is a partial word at the end of buf, handle it manually to
> >         * prevent overwriting past the end of buf
> >         */
> > -       if (rx_fifo_avail > 0 && buf_remaining > 0) {
> > -               BUG_ON(buf_remaining > 3);
> > +       bytes_to_transfer = atomic_read(&i2c_dev->msg_buf_remaining);

And again here...

> > +       if (rx_fifo_avail > 0 && bytes_to_transfer > 0) {
> > +               BUG_ON(bytes_to_transfer > 3);

That means that if msg_buf_remaining increases between those two reads,
this BUG_ON could trigger.

I assume this isn't possible, because the I2C core only sends one
transaction to the I2C driver and doesn't send any more requests down
until the previous is complete. If so, then the new code seems fine, but
I did want to double-check this.

Thanks.

> > +               atomic_set(&i2c_dev->msg_buf_remaining, 0);
> >                val = i2c_readl(i2c_dev, I2C_RX_FIFO);
> > -               memcpy(buf, &val, buf_remaining);
> > -               buf_remaining = 0;
> > +               memcpy(buf, &val, bytes_to_transfer);
> >                rx_fifo_avail--;
> >        }
> >
> > -       BUG_ON(rx_fifo_avail > 0 && buf_remaining > 0);
> > -       i2c_dev->msg_buf_remaining = buf_remaining;
> > +       BUG_ON(rx_fifo_avail > 0 &&
> > +               atomic_read(&i2c_dev->msg_buf_remaining) > 0);
> >        i2c_dev->msg_buf = buf;
> >        return 0;
> >  }

-- 
nvpublic

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/