[PATCH RFC 1/3] i2c: bcm2835: Avoid possible NULL ptr dereference

[Michael Zoran]

On Mon, 2017-02-20 at 22:22 +0100, Noralf Trønnes wrote:
> Den 20.02.2017 20.40, skrev Stefan Wahren:
> > Hi,
> > 
> > > Noralf Trønnes <noralf at tronnes.org> hat am 18. Februar 2017 um
> > > 19:34 geschrieben:
> > > 
> > > 
> > > 
> > > Den 16.02.2017 22.20, skrev Stefan Wahren:
> > > > Since commit e2474541032d ("bcm2835: Fix hang for writing
> > > > messages
> > > > larger than 16 bytes") the interrupt handler is prone to a
> > > > possible
> > > > NULL pointer dereference. This could happen if an interrupt
> > > > fires
> > > > before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly
> > > > occurs
> > > > on the RPi 3. Even this is an unexpected behavior the driver
> > > > must
> > > > handle that with an error instead of a crash.
> > > > 
> > > > CC: Noralf Trønnes <noralf at tronnes.org>
> > > > CC: Martin Sperl <kernel at martin.sperl.org>
> > > > Reported-by: Peter Robinson <pbrobinson at gmail.com>
> > > > Fixes: e2474541032d ("bcm2835: Fix hang for writing messages
> > > > larger than 16 bytes")
> > > > Signed-off-by: Stefan Wahren <stefan.wahren at i2se.com>
> > > > ---
> > > >    drivers/i2c/busses/i2c-bcm2835.c |    4 +++-
> > > >    1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/i2c/busses/i2c-bcm2835.c
> > > > b/drivers/i2c/busses/i2c-bcm2835.c
> > > > index c3436f6..10e39c8 100644
> > > > --- a/drivers/i2c/busses/i2c-bcm2835.c
> > > > +++ b/drivers/i2c/busses/i2c-bcm2835.c
> > > > @@ -195,7 +195,9 @@ static irqreturn_t bcm2835_i2c_isr(int
> > > > this_irq, void *data)
> > > >    	}
> > > >    
> > > >    	if (val & BCM2835_I2C_S_DONE) {
> > > > -		if (i2c_dev->curr_msg->flags & I2C_M_RD) {
> > > > +		if (!i2c_dev->curr_msg) {
> > > > +			dev_err(i2c_dev->dev, "Got unexpected
> > > > interrupt (from firmware?)\n");
> > > > +		} else if (i2c_dev->curr_msg->flags &
> > > > I2C_M_RD) {
> > > >    			bcm2835_drain_rxfifo(i2c_dev);
> > > >    			val = bcm2835_i2c_readl(i2c_dev,
> > > > BCM2835_I2C_S);
> > > >    		}
> > > 
> > > Thanks,
> > > 
> > > Acked-by: Noralf Trønnes <noralf at tronnes.org>
> > > 
> > 
> > thanks, but i would be more happier to receive feedback for patches
> > 2 and 3.
> 
> I have only worked on dts files downstream and never done any arm64
> stuff, so I'm not up to speed on this.
> 
> Noralf.
> 

Just a note, the original downstream driver returns IRQ_NONE in this
case.  Does that make any difference?

Also, the the original driver has the check at the start of the IRQ and
it doesn't log an error message.

I'm wondering if logging at err level is the best since it might make
people think a serious error has happened.  I think by default this
would show up on the console, correct?