linux-kernel - Re: [PATCH 1/1] sched: fix user

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] [day] [month] [year] [list]

Message-ID: <1c3ce8c4-354d-2f00-1dc6-e13cbe389828@redhat.com>
Date:   Tue, 22 Nov 2022 13:13:35 -0500
From:   Waiman Long <longman@...hat.com>
To:     Peter Zijlstra <peterz@...radead.org>, wangbiao3@...omi.com
Cc:     mingo@...hat.com, juri.lelli@...hat.com,
        vincent.guittot@...aro.org, brauner@...nel.org, bsegall@...gle.com,
        linux-kernel@...r.kernel.org, wenjieli@....qualcomm.com,
        chenguanyou@...omi.com, Will Deacon <will@...nel.org>
Subject: Re: [PATCH 1/1] sched: fix user_mask double free

On 11/22/22 10:39, Waiman Long wrote:
>
> On 11/22/22 09:05, Peter Zijlstra wrote:
>> So you failed:
>>
>>   - to Cc the original author of this code (Will Deacon)
>>   - to report what version this is against (apparently Linus' tree)
>>   - to check if this still applies to the latest tree (it doesn't)
>>   - to Cc the author of the code it now conflicts with (Waiman)
>>   - write something coherent in the changelog.
>>   - to include a Fixes tag.
>>
>> Still, let me try and make sense of things...
>>
>> On Mon, Nov 21, 2022 at 06:04:20PM +0800, wangbiao3@...omi.com wrote:
>>> From: wangbiao3 <wangbiao3@...omi.com>
>>>
>>> Clone/Fork a new task,call 
>>> dup_task_struct->arch_dup_task_struct(tsk,orig)
>>> which copy the data of parent/sibling task inclding p->user_cpus_ptr,so
>>> the user_cpus_ptr of newtask is the same with orig task's.When
>>> dup_task_struct call dup_user_cpus_ptr(tsk, orig, node),it return 0
>>> dircetly if src->user_cpus_ptris free by other task,in this case ,
>>> the newtask's address of user_cpus_ptr is not changed.
>> (even just inserting some whitespace would've made it so much easier to
>> read)
>>
>> But, the only way that would be possible is if
>> force_compatible_cpus_allowed_ptr() were to be called on !current, and
>> that just doesn't happen, the only callsite is:
>>
>> arch/arm64/kernel/process.c: force_compatible_cpus_allowed_ptr(current);
>>
>> And you can't be in fork() and exec() at the same time.
>>
>> If it were possible to call restrict_cpus_allowed_ptr() on a non-current
>> task then yes, absolutely, which is why:
>>
>>    8f9ea86fdf99 ("sched: Always preserve the user requested cpumask")
>>
>> also wraps the thing in pi_lock, but looking at it now, perhaps it needs
>> to do the alloc/copy first and swap under pi_lock instead.
>
> With the latest change, user_cpus_ptr, once set, will not be cleared 
> until when the task dies. That is why I don't recheck if user_cpus_ptr 
> is NULL under pi_lock. The user_cpus_ptr value can certainly changes 
> during its lifetime, but it will be stable under pi_lock. 
> clear_user_cpus_ptr() is called by release_user_cpus_ptr() only. As 
> said before, it is only call when the task dies at free_task() and so 
> there shouldn't be any other racing conditions that can happen at the 
> same time.

On second thought, do_set_cpus_allowed() can put NULL into 
user_cpus_ptr. So I think we should do null check in dup_user_cpus_ptr() 
inside the pi_lock. Will send a patch to do that.

Cheers,
Longman