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

[kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org]

> On 21.02.2017, at 09:58, Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org> wrote:
> 
> Am 20.02.2017 um 22:22 schrieb Noralf Trønnes:
>> 
>> Den 20.02.2017 20.40, skrev Stefan Wahren:
>>> Hi,
>>> 
>>>> Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.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-L59+Z2yzLopAfugRpC6u6w@public.gmane.org>
>>>>> CC: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
>>>>> Reported-by: Peter Robinson <pbrobinson-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>> Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes")
>>>>> Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
>>>>> ---
>>>>>   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-L59+Z2yzLopAfugRpC6u6w@public.gmane.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.
>> 
> 
> Okay, at the end i only want to know 2 things:
> 
> Does new message "Got unexpected interrupt" still appear after applying all 3 patches?
> 
> Does the annoying message "i2c-bcm2835 3f805000.i2c: i2c transfer timed out" still appear after applying all 3 patches?

I believe the issue is primarily that i2c0@3f805000 is 
“owned” by the firmware.

This is less of a problem on the rpi1 and rpi2, as there is 
little to no activity on those on i2c0 by the firmware
(besides when using the camera).

But on the rpi3 some Voltage sensors as well as GPIO 
extenders are attached to this bus, which are frequently
read/modified by the firmware.

So in principle this i2c bus should be disabled.
But if it is then there will be an interrupt triggered
on the VC4 core as well as on the ARM core - both will
see the interrupt.
but typically the RT-firmware will respond much faster
and disable the irq sources already before the arm gets
to “detect” the active interrupt in the irq handler.
But sometimes the firmware is not fast enough (or the irq
path is currently active in linux at the right time)
so the interrupt handler will trigger even if there
was no running i2c transfer on that bus.

The patch is basically defensive programming to avoid
such cases.

Martin


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html