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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87r1xqbx74.fsf@nanos.tec.linutronix.de>
Date:   Tue, 17 Mar 2020 20:25:35 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Ben Hutchings <ben@...adent.org.uk>,
        Edward Cree <ecree@...arflare.com>
Cc:     linux-kernel@...r.kernel.org, psodagud@...eaurora.org
Subject: Re: [PATCH] genirq: fix reference leaks on irq affinity notifiers

Ben Hutchings <ben@...adent.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@...adent.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.