[PATCH] genirq: call cancel_work_sync from irq_set_affinity_notifier

[PATCH] genirq: call cancel_work_sync from irq_set_affinity_notifier

[Prasad Sodagudi]

When ever notification of IRQ affinity changes, call
cancel_work_sync from irq_set_affinity_notifier to cancel
all pending works to avoid work list corruption.

Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
---
 kernel/irq/manage.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 9ec34a2..da8b2ee 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -356,6 +356,9 @@ static void irq_affinity_notify(struct work_struct *work)
 	desc->affinity_notify = notify;
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
 
+	if (!notify && old_notify)
+		cancel_work_sync(&old_notify->work);
+
 	if (old_notify)
 		kref_put(&old_notify->kref, old_notify->release);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH] genirq: call cancel_work_sync from irq_set_affinity_notifier

[Thomas Gleixner]

Prasad,

On Wed, 20 Mar 2019, Prasad Sodagudi wrote:

> Subject: [PATCH] genirq: call cancel_work_sync from irq_set_affinity_notifier

Please do not decribe WHAT the code change is. Give a consice explanation
WHY this change is done. The above is like '[PATCH] foo: Increment bar by 5'.

  [PATCH] genirq: Prevent UAF and work list corruption

> When ever notification of IRQ affinity changes, call
> cancel_work_sync from irq_set_affinity_notifier to cancel
> all pending works to avoid work list corruption.

Again, you describe first WHAT you are doing instead of telling WHY.

  When irq_set_affinity_notifier() replaces the notifier, then the
  reference count on the old notifier is dropped which causes it to be
  freed. But nothing ensures that the old notifier is not longer queued in
  the work list. If it is queued this results in a use after free and
  possibly in work list corruption.

  Ensure that the work is canceled before the reference is dropped.

See?

This gives precise context first and then describes the cure.

Also it is completely irrelevant whether this is achieved by calling
cancel_work_sync() or by something else. What matters is that it's
canceled. Changelogs describe context and concepts not implementation
details. The implementation details are in the patch itself.

> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
> ---
>  kernel/irq/manage.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 9ec34a2..da8b2ee 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -356,6 +356,9 @@ static void irq_affinity_notify(struct work_struct *work)
>  	desc->affinity_notify = notify;
>  	raw_spin_unlock_irqrestore(&desc->lock, flags);
>  
> +	if (!notify && old_notify)
> +		cancel_work_sync(&old_notify->work);

That '!notify' doesn't make any sense.

Thanks,

	tglx

Re: [PATCH] genirq: call cancel_work_sync from irq_set_affinity_notifier

[Sodagudi Prasad]

On 2019-03-21 09:19, Thomas Gleixner wrote:
> Prasad,
> 
> On Wed, 20 Mar 2019, Prasad Sodagudi wrote:
> 
>> Subject: [PATCH] genirq: call cancel_work_sync from 
>> irq_set_affinity_notifier
> 
> Please do not decribe WHAT the code change is. Give a consice 
> explanation
> WHY this change is done. The above is like '[PATCH] foo: Increment bar 
> by 5'.
> 
>   [PATCH] genirq: Prevent UAF and work list corruption
> 
>> When ever notification of IRQ affinity changes, call
>> cancel_work_sync from irq_set_affinity_notifier to cancel
>> all pending works to avoid work list corruption.
> 
> Again, you describe first WHAT you are doing instead of telling WHY.
> 
>   When irq_set_affinity_notifier() replaces the notifier, then the
>   reference count on the old notifier is dropped which causes it to be
>   freed. But nothing ensures that the old notifier is not longer queued 
> in
>   the work list. If it is queued this results in a use after free and
>   possibly in work list corruption.
> 
>   Ensure that the work is canceled before the reference is dropped.
> 
> See?

Hi Tglx,

Thanks for suggesting commit text and modifications.

> 
> This gives precise context first and then describes the cure.
> 
> Also it is completely irrelevant whether this is achieved by calling
> cancel_work_sync() or by something else. What matters is that it's
> canceled. Changelogs describe context and concepts not implementation
> details. The implementation details are in the patch itself.
> 
>> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
>> ---
>>  kernel/irq/manage.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>> index 9ec34a2..da8b2ee 100644
>> --- a/kernel/irq/manage.c
>> +++ b/kernel/irq/manage.c
>> @@ -356,6 +356,9 @@ static void irq_affinity_notify(struct work_struct 
>> *work)
>>  	desc->affinity_notify = notify;
>>  	raw_spin_unlock_irqrestore(&desc->lock, flags);
>> 
>> +	if (!notify && old_notify)
>> +		cancel_work_sync(&old_notify->work);
> 
> That '!notify' doesn't make any sense.

Yes. I will remove this in the next patch set.  Thanks for reviewing.

-thanks, Prasad
> 
> Thanks,
> 
> 	tglx

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
Linux Foundation Collaborative Project

[PATCH v2] genirq: Prevent use-after-free and work list corruption

[Prasad Sodagudi]

When irq_set_affinity_notifier() replaces the notifier, then the
reference count on the old notifier is dropped which causes it to be
freed. But nothing ensures that the old notifier is not longer queued
in the work list. If it is queued this results in a use after free and
possibly in work list corruption.

Ensure that the work is canceled before the reference is dropped.

Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
---
 kernel/irq/manage.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 9ec34a2..1a1ac84 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -356,8 +356,10 @@ static void irq_affinity_notify(struct work_struct *work)
 	desc->affinity_notify = notify;
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
 
-	if (old_notify)
+	if (old_notify) {
+		cancel_work_sync(&old_notify->work);
 		kref_put(&old_notify->kref, old_notify->release);
+	}
 
 	return 0;
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

[tip:irq/core] genirq: Prevent use-after-free and work list corruption

[tip-bot for Prasad Sodagudi]

Commit-ID:  59c39840f5abf4a71e1810a8da71aaccd6c17d26
Gitweb:     https://git.kernel.org/tip/59c39840f5abf4a71e1810a8da71aaccd6c17d26
Author:     Prasad Sodagudi <psodagud@codeaurora.org>
AuthorDate: Sun, 24 Mar 2019 07:57:04 -0700
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sun, 24 Mar 2019 22:13:17 +0100

genirq: Prevent use-after-free and work list corruption

When irq_set_affinity_notifier() replaces the notifier, then the
reference count on the old notifier is dropped which causes it to be
freed. But nothing ensures that the old notifier is not longer queued
in the work list. If it is queued this results in a use after free and
possibly in work list corruption.

Ensure that the work is canceled before the reference is dropped.

Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: marc.zyngier@arm.com
Link: https://lkml.kernel.org/r/1553439424-6529-1-git-send-email-psodagud@codeaurora.org

---
 kernel/irq/manage.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 1401afa0d58a..53a081392115 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -357,8 +357,10 @@ irq_set_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify)
 	desc->affinity_notify = notify;
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
 
-	if (old_notify)
+	if (old_notify) {
+		cancel_work_sync(&old_notify->work);
 		kref_put(&old_notify->kref, old_notify->release);
+	}
 
 	return 0;
 }