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

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, 27 May 2021 21:05:06 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Andy Lutomirski <luto@...nel.org>,
        syzbot <syzbot+2067e764dbcd10721e2e@...kaller.appspotmail.com>,
        bp@...en8.de, bp@...e.de, dave.hansen@...ux.intel.com,
        fenghua.yu@...el.com, hpa@...or.com, linux-kernel@...r.kernel.org,
        mingo@...hat.com, peterz@...radead.org,
        syzkaller-bugs@...glegroups.com, tony.luck@...el.com,
        x86@...nel.org, yu-cheng.yu@...el.com
Subject: Re: [syzbot] WARNING in ex_handler_fprestore

On Thu, May 27 2021 at 18:49, Thomas Gleixner wrote:
> On Thu, May 27 2021 at 00:03, Thomas Gleixner wrote:
> The original code kinda worked because fpu__clear() reinitialized the
> task fpu state, but as this code is preemptible the same issue can
> happen pre 5.8 as well if I'm not missing something. I'll verify that
> after dinner.

Yes, I was missing something and pre 5.8 _IS_ safe because fpu__clear()
wipes task->fpu.state. Preemption does not matter because
TIF_NEED_FPU_LOAD is set _before_ the copy from user happens and in the
failure case it is guaranteed that fpu__clear() is invoked before a
XRSTOR can happen from that wreckaged buffer.

So it's clearly this particular commit, which is sooo innocent according
to the submitter and fpu__clear() just helps to clean up the mess. It
just fails to clean up the mess which that commit created inside
fpu__clear() in the first place.

Clearly nothing of this whole system/user state seperation was
thoroughly tested, which means ANY patch which is XSTATE related is
blocked until this mess is analyzed and cleaned up.

>From now on ANY patch which extends or modifies XSTATE handling has to
be accompanied by proper test cases and analysis. Without that such
patches are not even going to be reviewed. This applies to all patches
in flight as well.

I don't mind bugs, but unprofessionally dismissing a conclusive bisect
and handwaving about ptrace modified segment registers on X86_64 is just
not acceptable.

Thanks,

        tglx