linux-kernel - Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit?

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: <CALCETrWBjvTiVBPc098cGSH2vdezvdg-i6N4kZjgdcPT7uXoiQ@mail.gmail.com>
Date:	Thu, 17 Dec 2015 18:32:14 -0800
From:	Andy Lutomirski <luto@...capital.net>
To:	Dave Hansen <dave.hansen@...ux.intel.com>
Cc:	Andy Lutomirski <luto@...nel.org>, Ingo Molnar <mingo@...nel.org>,
	Borislav Petkov <bp@...en8.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Brian Gerst <brgerst@...il.com>,
	Oleg Nesterov <oleg@...hat.com>
Subject: Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit?

On Thu, Dec 17, 2015 at 6:13 PM, Dave Hansen
<dave.hansen@...ux.intel.com> wrote:
> On 12/17/2015 05:48 PM, Andy Lutomirski wrote:
>> I think that, for PKRU in particular, we want the default signal
>> handling behavior to be a bit unusual.
>>
>> When a signal is delivered, I think we should save the entire xstate
>> including PKRU.  I see no reason to do anything other than that.
>
> Yep, agreed.
>
> But what about the register state when delivering a signal?  Don't we
> set the registers to the init state?  Do we need to preserve PKRU state
> instead of init'ing it?  The init state _is_ nice here because it is
> permissive allows us to do something useful no matter what PKRU gets set to.

I think we leave the extended regs alone.  Don't we?

I think that, for most signals, we want to leave PKRU as is,
especially for things that aren't SIGSEGV.  For SIGSEGV, maybe we want
an option to reset PKRU on delivery (and then set the flag to restore
on return?).

In any case, I think there are a decent number of programs out there
that use siglongjmp and therefore never actually hit sigreturn.  They
probably won't restore PKRU, so we shouldn't zero it out when
delivering most signals, I think.

>
> But, if we leave the init state in place when entering a delivering a
> signal, we _can't_ decide to (by default at least) preserve the
> in-signal state.
>
>> When a signal returns (sigreturn is called), though, I think we should
>> *not* restore PKRU.  To me, PKRU seems like a global per-thread state,
>> not something that signal handlers are likely to clobber and should
>> therefore have restored.  It's also unusual in that it doesn't fit
>> into the usual xstate feature model of being a bunch of registers that
>> are mostly caller-saved.
>>
>> Does this make sense?  Should we do this?
>
> Well, the signal handler isn't necessarily going to clobber it, but the
> delivery code already clobbers it when going to the init state.

Can you point to that code?

>
>> We have _fpx_sw_bytes.xfeatures and _xstate._header.xfeatures.  They
>> appear to do more or less the same thing.
>
> I thought the _fpx_sw_bytes were only for 32-bit (or FXSAVE/FXRSTOR?).

I thought they were everywhere.  fpu/signal.c looks that way to me.  I
could be missing something -- this code isn't the most straightforward
in the world.

>
>> Could we say that, for
>> certain new features (e.g. PKRU), if they're not in
>> _fpx_sw_bytes.xfeatures, then sigreturn will preserve the old content
>> rather than copying it?  User code that wants to change it on
>> sigreturn would manually or the feature in to xfeatures, which would
>> cause the feature to go to its init state if it's not in
>> _header.xfeatures or to go into the saved state if it is in
>> _header.xfeatures?
>
> I think we first need to decide on the state upon signal delivery.
>

Agreed.

> A practial problem at the moment is that we always call XRSTOR (aka
> copy_user_to_xregs()) with RFBM (aka 'mask') with all of the supported
> xfeatures.  So RFBM[i]=1 for each state, effectively.  A state with
> XSTATE_BV[i]=0 (aka header.xfeatures) and RFBM[i]=1 will init the state.
>
> We'd need to rig up the copy_user_to_xregs() to first read in
> header.xfeatures and then or RFBM with it.

Indeed.

>
> Not a huge deal, but something we want to think about, especially as it
> pertains to the init/modified optimizations.

Fair point.  FWIW, I don't think that sigreturn performance matters
all that much, so if we inadvertently lose some of the optimizations,
it may not be the end of the world.

I do wonder: are there any modern CPUs on which copy_user_to_xregs (as
opposed to first copying to the task_struct buffer and then
copy_kernel_to_xregs) is ever a win?  It avoids clobbering a few
cachelines and saves some memory bandwidth, but that may come at the
cost of disabling the init/modified optimizations when the subsequent
save on context switch hits a different VA/PA than the
copy_user_to_xregs used.

--Andy
--
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/