The handling of notify->work did not properly maintain notify->kref in two
cases:
1) where the work was already scheduled, another irq_set_affinity_locked()
would get the ref and (no-op-ly) schedule the work. Thus when
irq_affinity_notify() ran, it would drop the original ref but not the
additional one.
2) when cancelling the (old) work in irq_set_affinity_notifier(), if there
was outstanding work a ref had been got for it but was never put.
Fix both by checking the return values of the work handling functions
(schedule_work() for (1) and cancel_work_sync() for (2)) and put the
extra ref if the return value indicates preexisting work.
Fixes: cd7eab44e994 ("genirq: Add IRQ affinity notifiers")
Fixes: 59c39840f5ab ("genirq: Prevent use-after-free and work list corruption")
Signed-off-by: Edward Cree <[email protected]>
---
kernel/irq/manage.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 7eee98c38f25..b3aa1db895e6 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -323,7 +323,10 @@ int irq_set_affinity_locked(struct irq_data *data, const struct cpumask *mask,
if (desc->affinity_notify) {
kref_get(&desc->affinity_notify->kref);
- schedule_work(&desc->affinity_notify->work);
+ if (!schedule_work(&desc->affinity_notify->work))
+ /* Work was already scheduled, drop our extra ref */
+ kref_put(&desc->affinity_notify->kref,
+ desc->affinity_notify->release);
}
irqd_set(data, IRQD_AFFINITY_SET);
@@ -423,7 +426,9 @@ irq_set_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify)
raw_spin_unlock_irqrestore(&desc->lock, flags);
if (old_notify) {
- cancel_work_sync(&old_notify->work);
+ if (cancel_work_sync(&old_notify->work))
+ /* Pending work had a ref, put that one too */
+ kref_put(&old_notify->kref, old_notify->release);
kref_put(&old_notify->kref, old_notify->release);
}
On Fri, 2020-03-13 at 20:33 +0000, Edward Cree wrote:
> The handling of notify->work did not properly maintain notify->kref in two
> cases:
> 1) where the work was already scheduled, another irq_set_affinity_locked()
> would get the ref and (no-op-ly) schedule the work. Thus when
> irq_affinity_notify() ran, it would drop the original ref but not the
> additional one.
> 2) when cancelling the (old) work in irq_set_affinity_notifier(), if there
> was outstanding work a ref had been got for it but was never put.
This makes sense, but...
> Fix both by checking the return values of the work handling functions
> (schedule_work() for (1) and cancel_work_sync() for (2)) and put the
> extra ref if the return value indicates preexisting work.
>
> Fixes: cd7eab44e994 ("genirq: Add IRQ affinity notifiers")
> Fixes: 59c39840f5ab ("genirq: Prevent use-after-free and work list corruption")
> Signed-off-by: Edward Cree <[email protected]>
...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.
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.
Ben.
> ---
> kernel/irq/manage.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 7eee98c38f25..b3aa1db895e6 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -323,7 +323,10 @@ int irq_set_affinity_locked(struct irq_data *data, const struct cpumask *mask,
>
> if (desc->affinity_notify) {
> kref_get(&desc->affinity_notify->kref);
> - schedule_work(&desc->affinity_notify->work);
> + if (!schedule_work(&desc->affinity_notify->work))
> + /* Work was already scheduled, drop our extra ref */
> + kref_put(&desc->affinity_notify->kref,
> + desc->affinity_notify->release);
> }
> irqd_set(data, IRQD_AFFINITY_SET);
>
> @@ -423,7 +426,9 @@ irq_set_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify)
> raw_spin_unlock_irqrestore(&desc->lock, flags);
>
> if (old_notify) {
> - cancel_work_sync(&old_notify->work);
> + if (cancel_work_sync(&old_notify->work))
> + /* Pending work had a ref, put that one too */
> + kref_put(&old_notify->kref, old_notify->release);
> kref_put(&old_notify->kref, old_notify->release);
> }
>
--
Ben Hutchings
Humour is the best antidote to reality.
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.
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).
But I thought I'd bring it up in case it's halfway relevant.
-ed
[1] https://lore.kernel.org/lkml/[email protected]/T/#u
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 <[email protected]>
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.
Ben.
> But I thought I'd bring it up in case it's halfway relevant.
>
> -ed
>
> [1] https://lore.kernel.org/lkml/[email protected]/T/#u
--
Ben Hutchings
For every complex problem
there is a solution that is simple, neat, and wrong.
Ben Hutchings <[email protected]> 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 <[email protected]>
>
> 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.
The following commit has been merged into the irq/urgent branch of tip:
Commit-ID: df81dfcfd6991d547653d46c051bac195cd182c1
Gitweb: https://git.kernel.org/tip/df81dfcfd6991d547653d46c051bac195cd182c1
Author: Edward Cree <[email protected]>
AuthorDate: Fri, 13 Mar 2020 20:33:07
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Sat, 21 Mar 2020 17:32:46 +01:00
genirq: Fix reference leaks on irq affinity notifiers
The handling of notify->work did not properly maintain notify->kref in two
cases:
1) where the work was already scheduled, another irq_set_affinity_locked()
would get the ref and (no-op-ly) schedule the work. Thus when
irq_affinity_notify() ran, it would drop the original ref but not the
additional one.
2) when cancelling the (old) work in irq_set_affinity_notifier(), if there
was outstanding work a ref had been got for it but was never put.
Fix both by checking the return values of the work handling functions
(schedule_work() for (1) and cancel_work_sync() for (2)) and put the
extra ref if the return value indicates preexisting work.
Fixes: cd7eab44e994 ("genirq: Add IRQ affinity notifiers")
Fixes: 59c39840f5ab ("genirq: Prevent use-after-free and work list corruption")
Signed-off-by: Edward Cree <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Acked-by: Ben Hutchings <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
kernel/irq/manage.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 7eee98c..fe40c65 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -323,7 +323,11 @@ int irq_set_affinity_locked(struct irq_data *data, const struct cpumask *mask,
if (desc->affinity_notify) {
kref_get(&desc->affinity_notify->kref);
- schedule_work(&desc->affinity_notify->work);
+ if (!schedule_work(&desc->affinity_notify->work)) {
+ /* Work was already scheduled, drop our extra ref */
+ kref_put(&desc->affinity_notify->kref,
+ desc->affinity_notify->release);
+ }
}
irqd_set(data, IRQD_AFFINITY_SET);
@@ -423,7 +427,10 @@ irq_set_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify)
raw_spin_unlock_irqrestore(&desc->lock, flags);
if (old_notify) {
- cancel_work_sync(&old_notify->work);
+ if (cancel_work_sync(&old_notify->work)) {
+ /* Pending work had a ref, put that one too */
+ kref_put(&old_notify->kref, old_notify->release);
+ }
kref_put(&old_notify->kref, old_notify->release);
}