| 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
| ||
|
Date: Tue, 25 May 2021 18:24:10 +0800
From: "Xu, Yanfei" <yanfei.xu@...driver.com>
To: paulmck@...nel.org
Cc: Dmitry Vyukov <dvyukov@...gle.com>,
syzbot <syzbot+7b2b13f4943374609532@...kaller.appspotmail.com>,
rcu@...r.kernel.org, Andrew Morton <akpm@...ux-foundation.org>,
Andrii Nakryiko <andrii@...nel.org>,
Alexei Starovoitov <ast@...nel.org>,
Jens Axboe <axboe@...nel.dk>, bpf <bpf@...r.kernel.org>,
Christian Brauner <christian@...uner.io>,
Daniel Borkmann <daniel@...earbox.net>,
John Fastabend <john.fastabend@...il.com>,
Martin KaFai Lau <kafai@...com>,
KP Singh <kpsingh@...nel.org>,
LKML <linux-kernel@...r.kernel.org>,
netdev <netdev@...r.kernel.org>,
Shakeel Butt <shakeelb@...gle.com>,
Song Liu <songliubraving@...com>,
syzkaller-bugs <syzkaller-bugs@...glegroups.com>,
Yonghong Song <yhs@...com>
Subject: Re: [syzbot] KASAN: use-after-free Read in
check_all_holdout_tasks_trace
On 5/25/21 11:33 AM, Paul E. McKenney wrote:
> [Please note: This e-mail is from an EXTERNAL e-mail address]
>
> On Tue, May 25, 2021 at 10:31:55AM +0800, Xu, Yanfei wrote:
>>
>>
>> On 5/25/21 6:46 AM, Paul E. McKenney wrote:
>>> [Please note: This e-mail is from an EXTERNAL e-mail address]
>>>
>>> On Sun, May 23, 2021 at 09:13:50PM -0700, Paul E. McKenney wrote:
>>>> On Sun, May 23, 2021 at 08:51:56AM +0200, Dmitry Vyukov wrote:
>>>>> On Fri, May 21, 2021 at 7:29 PM syzbot
>>>>> <syzbot+7b2b13f4943374609532@...kaller.appspotmail.com> wrote:
>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> syzbot found the following issue on:
>>>>>>
>>>>>> HEAD commit: f18ba26d libbpf: Add selftests for TC-BPF management API
>>>>>> git tree: bpf-next
>>>>>> console output: https://syzkaller.appspot.com/x/log.txt?x=17f50d1ed00000
>>>>>> kernel config: https://syzkaller.appspot.com/x/.config?x=8ff54addde0afb5d
>>>>>> dashboard link: https://syzkaller.appspot.com/bug?extid=7b2b13f4943374609532
>>>>>>
>>>>>> Unfortunately, I don't have any reproducer for this issue yet.
>>>>>>
>>>>>> IMPORTANT: if you fix the issue, please add the following tag to the commit:
>>>>>> Reported-by: syzbot+7b2b13f4943374609532@...kaller.appspotmail.com
>>>>>
>>>>> This looks rcu-related. +rcu mailing list
>>>>
>>>> I think I see a possible cause for this, and will say more after some
>>>> testing and after becoming more awake Monday morning, Pacific time.
>>>
>>> No joy. From what I can see, within RCU Tasks Trace, the calls to
>>> get_task_struct() are properly protected (either by RCU or by an earlier
>>> get_task_struct()), and the calls to put_task_struct() are balanced by
>>> those to get_task_struct().
>>>
>>> I could of course have missed something, but at this point I am suspecting
>>> an unbalanced put_task_struct() has been added elsewhere.
>>>
>>> As always, extra eyes on this code would be a good thing.
>>>
>>> If it were reproducible, I would of course suggest bisection. :-/
>>>
>>> Thanx, Paul
>>>
>> Hi Paul,
>>
>> Could it be?
>>
>> CPU1 CPU2
>> trc_add_holdout(t, bhp)
>> //t->usage==2
>> release_task
>> put_task_struct_rcu_user
>> delayed_put_task_struct
>> ......
>> put_task_struct(t)
>> //t->usage==1
>>
>> check_all_holdout_tasks_trace
>> ->trc_wait_for_one_reader
>> ->trc_del_holdout
>> ->put_task_struct(t)
>> //t->usage==0 and task_struct freed
>> READ_ONCE(t->trc_reader_checked)
>> //ops， t had been freed.
>>
>> So, after excuting trc_wait_for_one_reader（）, task might had been removed
>> from holdout list and the corresponding task_struct was freed.
>> And we shouldn't do READ_ONCE(t->trc_reader_checked).
>
> I was suspicious of that call to trc_del_holdout() from within
> trc_wait_for_one_reader(), but the only time it executes is in the
> context of the current running task, which means that CPU 2 had better
> not be invoking release_task() on it just yet.
>
> Or am I missing your point?
Two times.
1. the task is current.
trc_wait_for_one_reader
->trc_del_holdout
2. task isn't current.
trc_wait_for_one_reader
->get_task_struct
->try_invoke_on_locked_down_task（trc_inspect_reader）
->trc_del_holdout
->put_task_struct
>
> Of course, if you can reproduce it, the following patch might be
Sorry...I can't reproduce it, just analyse syzbot's log. :(
Thanks,
Yanfei
> an interesting thing to try, my doubts notwithstanding. But more
> important, please check the patch to make sure that we are both
> talking about the same call to trc_del_holdout()!
>
> If we are talking about the same call to trc_del_holdout(), are you
> actually seeing that code execute except when rcu_tasks_trace_pertask()
> calls trc_wait_for_one_reader()?
>
>> I investigate the trc_wait_for_one_reader（） and found before we excute
>> trc_del_holdout, there is always set t->trc_reader_checked=true. How about
>> we just set the checked flag and unified excute trc_del_holdout()
>> in check_all_holdout_tasks_trace with checking the flag?
>
> The problem is that we cannot execute trc_del_holdout() except in
> the context of the RCU Tasks Trace grace-period kthread. So it is
> necessary to manipulate ->trc_reader_checked separately from the list
> in order to safely synchronize with IPIs and with the exit code path
> for any reader tasks, see for example trc_read_check_handler() and
> exit_tasks_rcu_finish_trace().
>
> Or are you thinking of some other approach?
>
> Thanx, Paul
>
> ------------------------------------------------------------------------
>
> diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> index efb8127f3a36..2a0d4bdd619a 100644
> --- a/kernel/rcu/tasks.h
> +++ b/kernel/rcu/tasks.h
> @@ -987,7 +987,6 @@ static void trc_wait_for_one_reader(struct task_struct *t,
> // The current task had better be in a quiescent state.
> if (t == current) {
> t->trc_reader_checked = true;
> - trc_del_holdout(t);
> WARN_ON_ONCE(READ_ONCE(t->trc_reader_nesting));
> return;
> }
>
Powered by blists - more mailing lists