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

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

[Edward Cree]

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 <ecree@solarflare.com>
---
 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);
 	}
 

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

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2596 bytes --]

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 <ecree@solarflare.com>

...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.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Edward Cree]

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/1553119211-29761-1-git-send-email-psodagud@codeaurora.org/T/#u

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

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2271 bytes --]

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.

Ben.

> But I thought I'd bring it up in case it's halfway relevant.
> 
> -ed
> 
> [1] https://lore.kernel.org/lkml/1553119211-29761-1-git-send-email-psodagud@codeaurora.org/T/#u
-- 
Ben Hutchings
For every complex problem
there is a solution that is simple, neat, and wrong.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Thomas Gleixner]

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.

[tip: irq/urgent] genirq: Fix reference leaks on irq affinity notifiers

[tip-bot2 for Edward Cree]

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 <ecree@solarflare.com>
AuthorDate:    Fri, 13 Mar 2020 20:33:07 
Committer:     Thomas Gleixner <tglx@linutronix.de>
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 <ecree@solarflare.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Ben Hutchings <ben@decadent.org.uk>
Link: https://lkml.kernel.org/r/24f5983f-2ab5-e83a-44ee-a45b5f9300f5@solarflare.com

---
 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);
 	}