linux-kernel - Re: [PATCH 01/16] 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: <20150422153245.GA22825@redhat.com>
Date:	Wed, 22 Apr 2015 17:32:45 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Borislav Petkov <bp@...en8.de>
Cc:	Dave Hansen <dave@...1.net>, linux-kernel@...r.kernel.org,
	x86@...nel.org, tglx@...utronix.de, dave.hansen@...ux.intel.com,
	riel@...hat.com, sbsiddha@...il.com, luto@...capital.net,
	mingo@...hat.com, hpa@...or.com, fenghua.yu@...el.com
Subject: Re: [PATCH 01/16] x86, fpu: wrap get_xsave_addr() to make it safer

On 04/22, Borislav Petkov wrote:
>
> On Wed, Apr 22, 2015 at 03:16:18PM +0200, Oleg Nesterov wrote:
> > I agree, tsk_used_math(tsk) looks better, simpy because we have this
> > argument.
> >
> > But this "tsk" should be always current, otherwise this code is wrong
>
> This is exactly what I'm asking: is that always the case?...

I can't look at these patches now, but iirc - yes. The caller is either
prctl() or exception. Dave will correct me.

Otherwise, once again, this code is simply buggy. So the comment should
probably explain this.

> > > Because used_math() is looking at current, maybe even in
> > > preemption-enabled paths - I'm eyeing task_get_bounds_dir() - and
> > > that current might get changed from under us and it might happen that
> > > current != tsk. Yes, no?
> >
> > Not sure I understand... "current" can't change from under us?
>
> ... I'm not sure all tsk_get_xsave_field() callers disable preemption.
> If not, then current can change from under us...

How? I am certainly missing you point... OK, please forge about FPU.
Consider this code:

	tsk = current;
	for (;;)
		BUG_ON(tsk != current);

it doesn't need to disable preemption. We do not care if CPU switches
to another thread, even if this thread executes the same code. Because
its tsk/current will differ, but "tsk == current" will be still true.

Could you please spell?

> > Even if this CPU switches to another thread which executes the same code,
> > that thread will obviously see another "current", but its "tsk" variable
> > will still match its "current".
>
> Well, we want to see if @tsk used math, not necessarily if current used
> math, especially if it is another task, right?

See above... used_math() should be correct because we know that tsk==current,
but I agree that tsk_used_math(tsk) looks better.

> I read tsk_get_xsave_field(@tsk, ) as give me the xsave field of @tsk
> but doing used_math() we're querying current and I'm not sure
>
> 	tsk == current
>
> in all the call sites of tsk_get_xsave_field().

Yes, the name/comment looks confusing a bit, as if you can use it when
tsk != current...

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/