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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
---
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
Commit-ID: 59c39840f5abf4a71e1810a8da71aaccd6c17d26
Gitweb: https://git.kernel.org/tip/59c39840f5abf4a71e1810a8da71aaccd6c17d26
Author: Prasad Sodagudi <[email protected]>
AuthorDate: Sun, 24 Mar 2019 07:57:04 -0700
Committer: Thomas Gleixner <[email protected]>
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 <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: [email protected]
Link: https://lkml.kernel.org/r/[email protected]
---
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;
}