linux-kernel - Re: [PATCH 01/17] x86, fpu: wrap get_xsave

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]

Message-ID: <20150325124533.GA17191@redhat.com>
Date:	Wed, 25 Mar 2015 13:45:33 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Andy Lutomirski <luto@...capital.net>
Cc:	Dave Hansen <dave.hansen@...el.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	X86 ML <x86@...nel.org>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	Rik van Riel <riel@...hat.com>,
	Suresh Siddha <sbsiddha@...il.com>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, Fenghua Yu <fenghua.yu@...el.com>
Subject: Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer

So far I do not understand this discussion ;) I didn't see the patches
and other emails...

On 03/24, Andy Lutomirski wrote:
>
> On Tue, Mar 24, 2015 at 5:12 PM, Dave Hansen <dave.hansen@...el.com> wrote:
> >>
> >> So why are you unlazying it?
> >
> > Oleg actually suggested it.

perhaps you are talking about math_error() ? It uses save_init_fpu()
in Linus's tree, but this is wrong. It was changed in tip/x86/fpu
to use unlazy_fpu() and save_init_fpu() was killed. Plus, just in
case, unlazy_fpu() was changed too and now it is

	void unlazy_fpu(struct task_struct *tsk)
	{
		preempt_disable();
		if (__thread_has_fpu(tsk)) {
			if (use_eager_fpu()) {
				__save_fpu(tsk);
			} else {
				__save_init_fpu(tsk);
				__thread_fpu_end(tsk);
			}
		}
		preempt_enable();
	}

and in fact (I think) it needs another change later, __thread_fpu_end()
should depend on __save_init_fpu().

> >> IIUC, the xstae for current can be in one of three logical states:
> >>
> >> 1. Live in CPU regs.  The in-memory copy is garbage and the state is
> >> in CPU regs.
> >> 2. Lazy.  The in-memory copy and the CPU regs match.  Writing to
> >> either copy is illegal.
> >> 3. In memory only.  Writing to the in-memory copy is safe.
> >>
> >> IIUC, you want to read the xstate, do you're okay with #2 or #3.  This
> >> would be tsk_get_xsave_field_for_read in my terminology.
> >>
> >> If you want to write the xstate, you'd need to be in state #3, which
> >> would be tsk_get_xsave_field_for_write.
> >>
> >> IIUC, unlazy_fpu just moves from from state 2 to 3.
> >
> > I won't completely claim to understand what's going on with the FPU
> > code, but I think your analysis is a bit off.
> >
> > unlazy_fpu() does __save_init_fpu() which (among other things) calls
> > xsave to dump the CPU registers to memory.  That doesn't make any sense
> > to do if "The in-memory copy and the CPU regs match."
> >
> > IOW, unlazy_fpu() is called when the in-memory copy is garbage and takes
> > us to a state where we can look at the in-memory copy.

Not necessarily, but most probably I misunderstood... If __thread_has_fpu()
we don't and can't know whether in-memory copy and the CPU regs match, so
we simply write FPU state to memory.

> I think that __save_init_fpu (called by unlazy_fpu) does that, but
> __thread_fpu_end calls __thread_clear_has_fpu, which, in turn, zaps
> fpu_owner_task, which will force an unnecessary xrstor.  Or maybe not
> if we have further bugs.

Yes, see above. We do not really need unconditional __thread_fpu_end().

> Holy crap these functions are poorly named.

Yes... let me quote another email from me:

	Perhaps you can also find a beter name for __save_init_fpu/etc ;) The
	name clearly suggests that it does "save + init" while in fact it does
	"save and may be destroy FPU state". At least for the callers, the fact
	that "destroy" is actually "init" doesn't really matter.

	But lets not rename it right now. This can conflict with the fixes we
	need to do first.

So please do not rename it now ;) I had to switch to other tasks, but I hope
I will continue the FPU cleanups "soon".

> Also, what, if anything,
> guarantees that fpu_owner_task is set on entry to userspace?

Well, this probaly needs other cleanups. I am not sure __thread_set_has_fpu()
actually should set fpu_owner_task = current. But this is offtopic.

Again, I am not sure I understand your concern... but fpu_owner_task should
be set if __thread_has_fpu(). Otherwise we do not care.

But __thread_fpu_end() must _clear_ fpu_owner_task, this is what we do care.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/