The driver's interrupt handler checks whether a message is currently
being handled with the curr_msg pointer. When it is NULL, the interrupt
is considered to be unexpected. Similarly, the i2c_start_transfer
routine checks for the remaining number of messages to handle in
num_msgs.
However, these values are never cleared and always keep the message and
number relevant to the latest transfer (which might be done already and
the underlying message memory might have been freed).
When an unexpected interrupt hits with the DONE bit set, the isr will
then try to access the flags field of the curr_msg structure, leading
to a fatal page fault.
Fix the issue by systematically clearing curr_msg and num_msgs in the
driver-wide device structure when a transfer is considered complete.
Signed-off-by: Paul Kocialkowski <[email protected]>
---
drivers/i2c/busses/i2c-bcm2835.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index 44deae78913e..5486252f5f2f 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -298,6 +298,9 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
return -ETIMEDOUT;
}
+ i2c_dev->curr_msg = NULL;
+ i2c_dev->num_msgs = 0;
+
if (!i2c_dev->msg_err)
return num;
--
2.20.1
On 12/21/18 4:11 AM, Paul Kocialkowski wrote:
> The driver's interrupt handler checks whether a message is currently
> being handled with the curr_msg pointer. When it is NULL, the interrupt
> is considered to be unexpected. Similarly, the i2c_start_transfer
> routine checks for the remaining number of messages to handle in
> num_msgs.
>
> However, these values are never cleared and always keep the message and
> number relevant to the latest transfer (which might be done already and
> the underlying message memory might have been freed).
>
> When an unexpected interrupt hits with the DONE bit set, the isr will
> then try to access the flags field of the curr_msg structure, leading
> to a fatal page fault.
>
> Fix the issue by systematically clearing curr_msg and num_msgs in the
> driver-wide device structure when a transfer is considered complete.
Should not this get a Fixes tag?
>
> Signed-off-by: Paul Kocialkowski <[email protected]>
> ---
> drivers/i2c/busses/i2c-bcm2835.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
> index 44deae78913e..5486252f5f2f 100644
> --- a/drivers/i2c/busses/i2c-bcm2835.c
> +++ b/drivers/i2c/busses/i2c-bcm2835.c
> @@ -298,6 +298,9 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> return -ETIMEDOUT;
> }
>
> + i2c_dev->curr_msg = NULL;
> + i2c_dev->num_msgs = 0;
> +
> if (!i2c_dev->msg_err)
> return num;
>
>
--
Florian
Hi Paul,
> Paul Kocialkowski <[email protected]> hat am 21. Dezember 2018 um 13:11 geschrieben:
>
>
> The driver's interrupt handler checks whether a message is currently
> being handled with the curr_msg pointer. When it is NULL, the interrupt
> is considered to be unexpected. Similarly, the i2c_start_transfer
> routine checks for the remaining number of messages to handle in
> num_msgs.
>
> However, these values are never cleared and always keep the message and
> number relevant to the latest transfer (which might be done already and
> the underlying message memory might have been freed).
>
> When an unexpected interrupt hits with the DONE bit set, the isr will
> then try to access the flags field of the curr_msg structure, leading
> to a fatal page fault.
>
> Fix the issue by systematically clearing curr_msg and num_msgs in the
> driver-wide device structure when a transfer is considered complete.
>
> Signed-off-by: Paul Kocialkowski <[email protected]>
> ---
> drivers/i2c/busses/i2c-bcm2835.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
> index 44deae78913e..5486252f5f2f 100644
> --- a/drivers/i2c/busses/i2c-bcm2835.c
> +++ b/drivers/i2c/busses/i2c-bcm2835.c
> @@ -298,6 +298,9 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> return -ETIMEDOUT;
> }
>
> + i2c_dev->curr_msg = NULL;
> + i2c_dev->num_msgs = 0;
AFAIU this would reduce the chance of a use-after-free dramatically but not completely.
Btw: Why did you leave of the i2c transfer timeout case?
Thanks
Stefan
> +
> if (!i2c_dev->msg_err)
> return num;
>
> --
> 2.20.1
>
Hi,
On Fri, 2018-12-21 at 09:52 -0800, Florian Fainelli wrote:
> On 12/21/18 4:11 AM, Paul Kocialkowski wrote:
> > The driver's interrupt handler checks whether a message is currently
> > being handled with the curr_msg pointer. When it is NULL, the interrupt
> > is considered to be unexpected. Similarly, the i2c_start_transfer
> > routine checks for the remaining number of messages to handle in
> > num_msgs.
> >
> > However, these values are never cleared and always keep the message and
> > number relevant to the latest transfer (which might be done already and
> > the underlying message memory might have been freed).
> >
> > When an unexpected interrupt hits with the DONE bit set, the isr will
> > then try to access the flags field of the curr_msg structure, leading
> > to a fatal page fault.
> >
> > Fix the issue by systematically clearing curr_msg and num_msgs in the
> > driver-wide device structure when a transfer is considered complete.
>
> Should not this get a Fixes tag?
Yes it totally should! Thanks for the suggestion.
Cheers,
Paul
> > Signed-off-by: Paul Kocialkowski <[email protected]>
> > ---
> > drivers/i2c/busses/i2c-bcm2835.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
> > index 44deae78913e..5486252f5f2f 100644
> > --- a/drivers/i2c/busses/i2c-bcm2835.c
> > +++ b/drivers/i2c/busses/i2c-bcm2835.c
> > @@ -298,6 +298,9 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> > return -ETIMEDOUT;
> > }
> >
> > + i2c_dev->curr_msg = NULL;
> > + i2c_dev->num_msgs = 0;
> > +
> > if (!i2c_dev->msg_err)
> > return num;
> >
> >
>
>
--
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com
Hi,
On Sat, 2018-12-22 at 13:19 +0100, Stefan Wahren wrote:
> Hi Paul,
>
> > Paul Kocialkowski <[email protected]> hat am 21. Dezember 2018 um 13:11 geschrieben:
> >
> >
> > The driver's interrupt handler checks whether a message is currently
> > being handled with the curr_msg pointer. When it is NULL, the interrupt
> > is considered to be unexpected. Similarly, the i2c_start_transfer
> > routine checks for the remaining number of messages to handle in
> > num_msgs.
> >
> > However, these values are never cleared and always keep the message and
> > number relevant to the latest transfer (which might be done already and
> > the underlying message memory might have been freed).
> >
> > When an unexpected interrupt hits with the DONE bit set, the isr will
> > then try to access the flags field of the curr_msg structure, leading
> > to a fatal page fault.
> >
> > Fix the issue by systematically clearing curr_msg and num_msgs in the
> > driver-wide device structure when a transfer is considered complete.
> >
> > Signed-off-by: Paul Kocialkowski <[email protected]>
> > ---
> > drivers/i2c/busses/i2c-bcm2835.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
> > index 44deae78913e..5486252f5f2f 100644
> > --- a/drivers/i2c/busses/i2c-bcm2835.c
> > +++ b/drivers/i2c/busses/i2c-bcm2835.c
> > @@ -298,6 +298,9 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> > return -ETIMEDOUT;
> > }
> >
> > + i2c_dev->curr_msg = NULL;
> > + i2c_dev->num_msgs = 0;
>
> AFAIU this would reduce the chance of a use-after-free dramatically but not completely.
Do you have a specific case of use-after-free related to these
variables in mind that this cleanup would not fix (except for the
timeout case)?
The msg_buf element (in association with msg_buf_remaining keeping its
previous value) could also lead to similar problems, so I'm thinking of
making a bcm2835_i2c_finish_transfer function to group all the cleanups
and call that right after wait_for_completion_timeout.
> Btw: Why did you leave of the i2c transfer timeout case?
I should definitely have included it, actually.
Cheers,
Paul
> Thanks
> Stefan
>
> > +
> > if (!i2c_dev->msg_err)
> > return num;
> >
> > --
> > 2.20.1
> >
--
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com
Hi Paul,
> Paul Kocialkowski <[email protected]> hat am 24. Dezember 2018 um 10:10 geschrieben:
>
>
> Hi,
>
> On Sat, 2018-12-22 at 13:19 +0100, Stefan Wahren wrote:
> > Hi Paul,
> >
> > > Paul Kocialkowski <[email protected]> hat am 21. Dezember 2018 um 13:11 geschrieben:
> > >
> > >
> > > The driver's interrupt handler checks whether a message is currently
> > > being handled with the curr_msg pointer. When it is NULL, the interrupt
> > > is considered to be unexpected. Similarly, the i2c_start_transfer
> > > routine checks for the remaining number of messages to handle in
> > > num_msgs.
> > >
> > > However, these values are never cleared and always keep the message and
> > > number relevant to the latest transfer (which might be done already and
> > > the underlying message memory might have been freed).
> > >
> > > When an unexpected interrupt hits with the DONE bit set, the isr will
> > > then try to access the flags field of the curr_msg structure, leading
> > > to a fatal page fault.
> > >
> > > Fix the issue by systematically clearing curr_msg and num_msgs in the
> > > driver-wide device structure when a transfer is considered complete.
> > >
> > > Signed-off-by: Paul Kocialkowski <[email protected]>
> > > ---
> > > drivers/i2c/busses/i2c-bcm2835.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
> > > index 44deae78913e..5486252f5f2f 100644
> > > --- a/drivers/i2c/busses/i2c-bcm2835.c
> > > +++ b/drivers/i2c/busses/i2c-bcm2835.c
> > > @@ -298,6 +298,9 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> > > return -ETIMEDOUT;
> > > }
> > >
> > > + i2c_dev->curr_msg = NULL;
> > > + i2c_dev->num_msgs = 0;
> >
> > AFAIU this would reduce the chance of a use-after-free dramatically but not completely.
>
> Do you have a specific case of use-after-free related to these
> variables in mind that this cleanup would not fix (except for the
> timeout case)?
okay i was wrong about the use-after-free. But i'm not sure about this scenario on the I2C bus shared with the VC4:
1. ARM core starts I2C transfer (bcm2835_i2c_start_transfer)
2. VC4 triggers a BCM2835_I2C_S_DONE interrupt
3. ARM core catches this interrupt before the VC4
Stefan
>
> The msg_buf element (in association with msg_buf_remaining keeping its
> previous value) could also lead to similar problems, so I'm thinking of
> making a bcm2835_i2c_finish_transfer function to group all the cleanups
> and call that right after wait_for_completion_timeout.
>
> > Btw: Why did you leave of the i2c transfer timeout case?
>
> I should definitely have included it, actually.
>
> Cheers,
>
> Paul
>
> > Thanks
> > Stefan
> >
> > > +
> > > if (!i2c_dev->msg_err)
> > > return num;
> > >
> > > --
> > > 2.20.1
> > >
> --
> Paul Kocialkowski, Bootlin (formerly Free Electrons)
> Embedded Linux and kernel engineering
> https://bootlin.com
>
Hi Stefan,
On Thu, 2018-12-27 at 15:05 +0100, Stefan Wahren wrote:
> Hi Paul,
>
> > Paul Kocialkowski <[email protected]> hat am 24. Dezember 2018 um 10:10 geschrieben:
> >
> >
> > Hi,
> >
> > On Sat, 2018-12-22 at 13:19 +0100, Stefan Wahren wrote:
> > > Hi Paul,
> > >
> > > > Paul Kocialkowski <[email protected]> hat am 21. Dezember 2018 um 13:11 geschrieben:
> > > >
> > > >
> > > > The driver's interrupt handler checks whether a message is currently
> > > > being handled with the curr_msg pointer. When it is NULL, the interrupt
> > > > is considered to be unexpected. Similarly, the i2c_start_transfer
> > > > routine checks for the remaining number of messages to handle in
> > > > num_msgs.
> > > >
> > > > However, these values are never cleared and always keep the message and
> > > > number relevant to the latest transfer (which might be done already and
> > > > the underlying message memory might have been freed).
> > > >
> > > > When an unexpected interrupt hits with the DONE bit set, the isr will
> > > > then try to access the flags field of the curr_msg structure, leading
> > > > to a fatal page fault.
> > > >
> > > > Fix the issue by systematically clearing curr_msg and num_msgs in the
> > > > driver-wide device structure when a transfer is considered complete.
> > > >
> > > > Signed-off-by: Paul Kocialkowski <[email protected]>
> > > > ---
> > > > drivers/i2c/busses/i2c-bcm2835.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
> > > > index 44deae78913e..5486252f5f2f 100644
> > > > --- a/drivers/i2c/busses/i2c-bcm2835.c
> > > > +++ b/drivers/i2c/busses/i2c-bcm2835.c
> > > > @@ -298,6 +298,9 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> > > > return -ETIMEDOUT;
> > > > }
> > > >
> > > > + i2c_dev->curr_msg = NULL;
> > > > + i2c_dev->num_msgs = 0;
> > >
> > > AFAIU this would reduce the chance of a use-after-free dramatically but not completely.
> >
> > Do you have a specific case of use-after-free related to these
> > variables in mind that this cleanup would not fix (except for the
> > timeout case)?
>
> okay i was wrong about the use-after-free. But i'm not sure about this scenario on the I2C bus shared with the VC4:
>
> 1. ARM core starts I2C transfer (bcm2835_i2c_start_transfer)
> 2. VC4 triggers a BCM2835_I2C_S_DONE interrupt
> 3. ARM core catches this interrupt before the VC4
Well, I thought the VC4 has precedence over the ARM core, so if the
interrupts hits in hardware, it should be seen by the VC4 first and
forwarded to the ARM core if needed (I might be wrong though). So in
that case, perhaps the ARM core wouldn't see the interrupt and just
timeout?
Either way, I'm don't think that having both the VC4 and the ARM core
poke on the same bus is a scenario we can realisticly aim to support.
Although it should definitely not make our driver crash if that
happens.
The HDMI DDC bus is an I2C bus shared between the ARM core and VC4, but
the VC4 firmware should detect from the device-tree that Linux will
attempt to drive the bus and let the ARM core deal with it without
interference.
Thanks for your feedback!
Cheers,
Paul
> > The msg_buf element (in association with msg_buf_remaining keeping its
> > previous value) could also lead to similar problems, so I'm thinking of
> > making a bcm2835_i2c_finish_transfer function to group all the cleanups
> > and call that right after wait_for_completion_timeout.
> >
> > > Btw: Why did you leave of the i2c transfer timeout case?
> >
> > I should definitely have included it, actually.
> >
> > Cheers,
> >
> > Paul
> >
> > > Thanks
> > > Stefan
> > >
> > > > +
> > > > if (!i2c_dev->msg_err)
> > > > return num;
> > > >
> > > > --
> > > > 2.20.1
> > > >
> > --
> > Paul Kocialkowski, Bootlin (formerly Free Electrons)
> > Embedded Linux and kernel engineering
> > https://bootlin.com
> >
--
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com
Stefan Wahren <[email protected]> writes:
> Hi Paul,
>
>> Paul Kocialkowski <[email protected]> hat am 24. Dezember 2018 um 10:10 geschrieben:
>>
>>
>> Hi,
>>
>> On Sat, 2018-12-22 at 13:19 +0100, Stefan Wahren wrote:
>> > Hi Paul,
>> >
>> > > Paul Kocialkowski <[email protected]> hat am 21. Dezember 2018 um 13:11 geschrieben:
>> > >
>> > >
>> > > The driver's interrupt handler checks whether a message is currently
>> > > being handled with the curr_msg pointer. When it is NULL, the interrupt
>> > > is considered to be unexpected. Similarly, the i2c_start_transfer
>> > > routine checks for the remaining number of messages to handle in
>> > > num_msgs.
>> > >
>> > > However, these values are never cleared and always keep the message and
>> > > number relevant to the latest transfer (which might be done already and
>> > > the underlying message memory might have been freed).
>> > >
>> > > When an unexpected interrupt hits with the DONE bit set, the isr will
>> > > then try to access the flags field of the curr_msg structure, leading
>> > > to a fatal page fault.
>> > >
>> > > Fix the issue by systematically clearing curr_msg and num_msgs in the
>> > > driver-wide device structure when a transfer is considered complete.
>> > >
>> > > Signed-off-by: Paul Kocialkowski <[email protected]>
>> > > ---
>> > > drivers/i2c/busses/i2c-bcm2835.c | 3 +++
>> > > 1 file changed, 3 insertions(+)
>> > >
>> > > diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
>> > > index 44deae78913e..5486252f5f2f 100644
>> > > --- a/drivers/i2c/busses/i2c-bcm2835.c
>> > > +++ b/drivers/i2c/busses/i2c-bcm2835.c
>> > > @@ -298,6 +298,9 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
>> > > return -ETIMEDOUT;
>> > > }
>> > >
>> > > + i2c_dev->curr_msg = NULL;
>> > > + i2c_dev->num_msgs = 0;
>> >
>> > AFAIU this would reduce the chance of a use-after-free dramatically but not completely.
>>
>> Do you have a specific case of use-after-free related to these
>> variables in mind that this cleanup would not fix (except for the
>> timeout case)?
>
> okay i was wrong about the use-after-free. But i'm not sure about this scenario on the I2C bus shared with the VC4:
>
> 1. ARM core starts I2C transfer (bcm2835_i2c_start_transfer)
> 2. VC4 triggers a BCM2835_I2C_S_DONE interrupt
> 3. ARM core catches this interrupt before the VC4
We don't share I2C buses with VC4, though, right? That would just be
totally broken.
> Eric Anholt <[email protected]> hat am 27. Dezember 2018 um 19:15 geschrieben:
>
>
> Stefan Wahren <[email protected]> writes:
>
> > Hi Paul,
> >
> >> Paul Kocialkowski <[email protected]> hat am 24. Dezember 2018 um 10:10 geschrieben:
> >>
> >>
> >> Hi,
> >>
> >> On Sat, 2018-12-22 at 13:19 +0100, Stefan Wahren wrote:
> >> > Hi Paul,
> >> >
> >> > > Paul Kocialkowski <[email protected]> hat am 21. Dezember 2018 um 13:11 geschrieben:
> >> > >
> >> > >
> >> > > The driver's interrupt handler checks whether a message is currently
> >> > > being handled with the curr_msg pointer. When it is NULL, the interrupt
> >> > > is considered to be unexpected. Similarly, the i2c_start_transfer
> >> > > routine checks for the remaining number of messages to handle in
> >> > > num_msgs.
> >> > >
> >> > > However, these values are never cleared and always keep the message and
> >> > > number relevant to the latest transfer (which might be done already and
> >> > > the underlying message memory might have been freed).
> >> > >
> >> > > When an unexpected interrupt hits with the DONE bit set, the isr will
> >> > > then try to access the flags field of the curr_msg structure, leading
> >> > > to a fatal page fault.
> >> > >
> >> > > Fix the issue by systematically clearing curr_msg and num_msgs in the
> >> > > driver-wide device structure when a transfer is considered complete.
> >> > >
> >> > > Signed-off-by: Paul Kocialkowski <[email protected]>
> >> > > ---
> >> > > drivers/i2c/busses/i2c-bcm2835.c | 3 +++
> >> > > 1 file changed, 3 insertions(+)
> >> > >
> >> > > diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
> >> > > index 44deae78913e..5486252f5f2f 100644
> >> > > --- a/drivers/i2c/busses/i2c-bcm2835.c
> >> > > +++ b/drivers/i2c/busses/i2c-bcm2835.c
> >> > > @@ -298,6 +298,9 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> >> > > return -ETIMEDOUT;
> >> > > }
> >> > >
> >> > > + i2c_dev->curr_msg = NULL;
> >> > > + i2c_dev->num_msgs = 0;
> >> >
> >> > AFAIU this would reduce the chance of a use-after-free dramatically but not completely.
> >>
> >> Do you have a specific case of use-after-free related to these
> >> variables in mind that this cleanup would not fix (except for the
> >> timeout case)?
> >
> > okay i was wrong about the use-after-free. But i'm not sure about this scenario on the I2C bus shared with the VC4:
> >
> > 1. ARM core starts I2C transfer (bcm2835_i2c_start_transfer)
> > 2. VC4 triggers a BCM2835_I2C_S_DONE interrupt
> > 3. ARM core catches this interrupt before the VC4
>
> We don't share I2C buses with VC4, though, right? That would just be
> totally broken.
All i remember is this comment on Github:
https://github.com/anholt/linux/issues/89#issuecomment-280111496
and currently all 3 I2C busses are enabled for the ARM core in bcm2835-rpi.dtsi.
Stefan