linux-kernel - Re: [syzbot] WARNING in kthread_is_per

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:   Tue, 20 Apr 2021 10:43:43 +0100
From:   Valentin Schneider <valentin.schneider@....com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     syzbot <syzbot+9362b31a2e0cad8b749d@...kaller.appspotmail.com>,
        bp@...en8.de, dwmw@...zon.co.uk, hpa@...or.com,
        linux-kernel@...r.kernel.org, luto@...nel.org, mingo@...hat.com,
        syzkaller-bugs@...glegroups.com, tglx@...utronix.de, x86@...nel.org
Subject: Re: [syzbot] WARNING in kthread_is_per_cpu

On 20/04/21 10:51, Peter Zijlstra wrote:
> On Mon, Apr 19, 2021 at 08:58:26PM +0100, Valentin Schneider wrote:
>
>> Looks about right, IIUC the key being:
>>
>>   p->flags & PF_KTHREAD + p->set_child_tid => the struct kthread is
>>   persistent
>>
>>   p->flags & PF_KTHREAD => you may or may not have a struct kthread (see
>>   kernel/umh.c kernel_thread() uses). PF_KTHREAD isn't even guaranteed to
>>   persist (begin_new_exec()), which seems to be what the syzbot hit.
>
> Ack, that's nicely put.
>
>> While we're at it, does free_kthread_struct() want the __to_kthread()
>> treatment as well? The other to_kthread() callsites looked like they only
>> made sense with a "proper" kthread anyway.
>
> I think free_kthread_struct() is ok, because a task at that point in its
> lifetime cannot be also doing exec().
>

What if it's one of those kthreads created by directly invoking
kernel_thread()? AFAICT right now it's only umh, and that one does execve()
so it ends up stripped of PF_KTHREAD. It could however go through an error
path, i.e. not call exec, and exit, giving us:

  put_task_struct(p)
  `\
    free_task(p)
    `\
      if (tsk->flags & PF_KTHREAD)
          free_kthread_struct(tsk);
          `\
            to_kthread(p)

> kthread_func() is another 'fun' trainwreck waiting to happen -- luckily
> the only caller uses current, still let me go fix it.
>
> kthread_probe_data() relies on PF_WQ_WORKER implying PF_KTHREAD but
> otherwise seems very fragile too.
>
> Something like so then?
>

Other than the above:

Reviewed-by: Valentin Schneider <Valentin.Schneider@....com>