• Thomas Gleixner's avatar
    genirq: Validate action before dereferencing it in handle_irq_event_percpu() · 570540d5
    Thomas Gleixner authored
    commit 71f64340 changed the handling of irq_desc->action from
    
    CPU 0                   CPU 1
    free_irq()              lock(desc)
      lock(desc)            handle_edge_irq()
                            if (desc->action) {
                              handle_irq_event()
                                action = desc->action
                                unlock(desc)
      desc->action = NULL       handle_irq_event_percpu(desc, action)
                                  action->xxx
    to
    
    CPU 0                   CPU 1
    free_irq()              lock(desc)
      lock(desc)            handle_edge_irq()
                            if (desc->action) {
                              handle_irq_event()
                                unlock(desc)
      desc->action = NULL       handle_irq_event_percpu(desc, action)
                                  action = desc->action
                                  action->xxx
    
    So if free_irq manages to set the action to NULL between the unlock and before
    the readout, we happily dereference a null pointer.
    
    We could simply revert 71f64340, but we want to preserve the better code
    generation. A simple solution is to change the action loop from a do {} while
    to a while {} loop.
    
    This is safe because we either see a valid desc->action or NULL. If the action
    is about to be removed it is still valid as free_irq() is blocked on
    synchronize_irq().
    
    CPU 0                   CPU 1
    free_irq()              lock(desc)
      lock(desc)            handle_edge_irq()
                              handle_irq_event(desc)
                                set(INPROGRESS)
                                unlock(desc)
                                handle_irq_event_percpu(desc)
                                action = desc->action
      desc->action = NULL           while (action) {
                                      action->xxx
                                      ...
                                      action = action->next;
      sychronize_irq()
        while(INPROGRESS);      lock(desc)
                                clr(INPROGRESS)
    free(action)
    
    That's basically the same mechanism as we have for shared
    interrupts. action->next can become NULL while handle_irq_event_percpu()
    runs. Either it sees the action or NULL. It does not matter, because action
    itself cannot go away before the interrupt in progress flag has been cleared.
    
    Fixes: commit 71f64340 "genirq: Remove the second parameter from handle_irq_event_percpu()"
    Reported-by: zyjzyj2000@gmail.com
    Signed-off-by: default avatarThomas Gleixner <tglx@linutronix.de>
    Cc: Huang Shijie <shijie.huang@arm.com>
    Cc: Jiang Liu <jiang.liu@linux.intel.com>
    Cc: Peter Zijlstra <peterz@infradead.org>
    Cc: stable@vger.kernel.org
    Link: http://lkml.kernel.org/r/alpine.DEB.2.11.1601131224190.3575@nanos
    570540d5
handle.c 5.13 KB