linux-kernel - Re: [PATCH] genirq: call cancel_work_sync from irq_set_affinity

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 21 Mar 2019 17:19:39 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Prasad Sodagudi <psodagud@...eaurora.org>
cc:     LKML <linux-kernel@...r.kernel.org>,
        Marc Zyngier <marc.zyngier@....com>
Subject: Re: [PATCH] genirq: call cancel_work_sync from
 irq_set_affinity_notifier

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