Re: [PATCH] genirq: fix reference leaks on irq affinity notifiers

[Thomas Gleixner <tglx@linutronix.de>]

Ben Hutchings <ben@decadent.org.uk> writes:
> On Tue, 2020-03-17 at 10:58 +0000, Edward Cree wrote:
>> On 15/03/2020 20:29, Ben Hutchings wrote:
>> > ...since the pending work item holds a reference to the notification
>> > state, it's still not clear to me why or whether "genirq: Prevent use-
>> > after-free and work list corruption" was needed.
>> Yeah, I think that commit was bogus.  The email thread[1] doesn't
>>  exactly inspire confidence either.  I think the submitter just didn't
>>  realise that there was a ref corresponding to the work; AFAICT there's
>>  no way the alleged "work list corruption" could happen.
>> 
>> > If it's reasonable to cancel_work_sync() when removing a notifier, I
>> > think we can remove the kref and call the release function directly.
>> I'd prefer to stick to the smaller fix for -rc and stable.  But if you
>>  want to remove the kref for -next, I'd be happy to Ack that patch.
>
> OK, then you can add:
>
> Acked-by: Ben Hutchings <ben@decadent.org.uk>
>
> to this one.
>
>> Btw, we (sfc linux team) think there's still a use-after-free issue in
>>  the cpu_rmap lib, as follows:
>> 1) irq_cpu_rmap_add creates a glue and notifier, adds glue to rmap->obj
>> 2) someone else does irq_set_affinity_notifier.
>>    This causes cpu_rmap's notifier (old_notify) to get released, and so
>>    irq_cpu_rmap_release kfrees glue.  But it's still in rmap->obj
>> 3) free_irq_cpu_rmap loops over obj, finds the glue, tries to clear its
>>    notifier.
>> Now one could say that this UAF is academic, since having two bits of
>>  code trying to register notifiers for the same IRQ is broken anyway
>>  (in this case, the rmap would stop getting updated, because the
>>  "someone else" stole the notifier).
>
> So far as I can remember, my thinking was that only non-shared IRQs
> will have notifiers and only the current user of the IRQ will set the
> notifier.  The doc comment for irq_set_affinity_notifier() implies the
> latter restriction, but it might be worth spelling this out explicitly.

Bah. I so wish these notifiers would have never been introduced at all.