Re: null pointer dereference error in timer-sun5i.c

Re: null pointer dereference error in timer-sun5i.c

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1535 bytes --]

Hi,

On Mon, Feb 16, 2015 at 04:36:06PM +0900, 박용배 wrote:
> Hello. My name is Yongbae Park.
> 
> I would like to report a possible null pointer dereference error at
> sun5i_timer_interrupt() in drivers/clocksource/timer-sun5i.c (version:
> 3.19-rc5). The null pointer dereference error occurs if the interrupt
> handler sun5i_timer_interrupt() accesses evt->event_handler (line 128) when
> evt->event_handler is null and not defined by sun5i_timer_init().
> 
> sun5i_timer_init() first registers sun5i_timer_interrupt() as the interrupt
> handler at line 181, and then defines the clockevent handler at line 192.
> As a consequence, the interrupt handler can be executed before the
> clockevent handler definition when an interrupt occurs between line 181 and
> line 192. The detail error scenario is the following:

That's very true. Thanks for reporting it.

However, this shouldn't really happen in real life, since the hstimer
are never used by the bootloader (which means that we don't have a
running timer already), and that this isn't the default timer as well
(so we don't program it either).

The only case where this could happen (in the default case), would be
a spurious interrupt.

Did you encounter this bug in real life?

Would you care to make a patch for this issue, similar to the patches
you pointed at, since you're the one who found this issue?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

null pointer dereference error in timer-sun5i.c

[박용배]

Hello. My name is Yongbae Park.


I would like to report a possible null pointer dereference error at
sun5i_timer_interrupt() in drivers/clocksource/timer-sun5i.c (version:
3.19-rc5). The null pointer dereference error occurs if the interrupt
handler sun5i_timer_interrupt() accesses evt->event_handler (line 128)
when evt->event_handler is null and not defined by sun5i_timer_init().

sun5i_timer_init() first registers sun5i_timer_interrupt() as the
interrupt handler at line 181, and then defines the clockevent handler
at line 192. As a consequence, the interrupt handler can be executed
before the clockevent handler definition when an interrupt occurs
between line 181 and line 192. The detail error scenario is the
following:

145: static void __init sun5i_timer_init(struct device_node *node) {
...
181: ret = setup_irq(irq, &sun5i_timer_irq);d
...
------ An interrupt is fired and the interrupt handler is called -------
    123: static irqreturn_t sun5i_timer_interrupt(int irq, void *dev_id)
    124: {
    125:   struct clock_event_device *evt = (struct clock_event_device *)dev_id;
    126:
    127:   writel(0x1, timer_base + TIMER_IRQ_ST_REG);
    128:   evt->event_handler(evt); // evt->event_handler is not defined
    129:
    130:   return IRQ_HANDLED;
    131: }
------ The execution of the interrupt handler is finished ------
...
192: clockevents_config_and_register(&sun5i_clockevent, rate,
193:             TIMER_SYNC_TICKS, 0xffffffff);

To resolve the problem, I think that the interrupt handler should be
registered after the clock handler registration.

For your information, I give you the references to similar issues from
the previous bug reports:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6bab4a8a1888729f17f4923cc5867e4674f66333
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=da64c2a8dee66ca03f4f3e15d84be7bedf73db3d

Thank you.