lkml.org 
[lkml]   [2020]   [Mar]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] genirq: fix reference leaks on irq affinity notifiers
Date
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.

\
 
 \ /
  Last update: 2020-03-17 20:27    [W:0.026 / U:0.288 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site