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 13:31:33 -0700
From:   Sodagudi Prasad <psodagud@...eaurora.org>
To:     Thomas Gleixner <tglx@...utronix.de>
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

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

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