Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Andy Lutomirski <luto@kernel.org>,
        syzbot <syzbot+2067e764dbcd10721e2e@syzkaller.appspotmail.com>,
        bp@alien8.de, bp@suse.de, dave.hansen@linux.intel.com,
        fenghua.yu@intel.com, hpa@zytor.com, linux-kernel@vger.kernel.org,
        mingo@redhat.com, peterz@infradead.org,
        syzkaller-bugs@googlegroups.com, tony.luck@intel.com,
        x86@kernel.org, yu-cheng.yu@intel.com
Subject: Re: [syzbot] WARNING in ex_handler_fprestore
In-Reply-To: <87bl8w86m3.ffs@nanos.tec.linutronix.de>
References: <0000000000004c453905c30f8334@google.com> <fe20f360-7c6c-f1bf-c3a5-403ae979fae6@kernel.org> <87sg29886g.ffs@nanos.tec.linutronix.de> <87bl8w86m3.ffs@nanos.tec.linutronix.de>
Date:   Thu, 27 May 2021 21:05:06 +0200
Message-ID: <874keo80bh.ffs@nanos.tec.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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