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

[Andy Lutomirski]

On Mon, Dec 21, 2015 at 3:07 PM, Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:
> On Mon, Dec 21, 2015 at 3:04 PM, Dave Hansen
> <dave.hansen@xxxxxxxxxxxxxxx> wrote:
>> On 12/18/2015 02:28 PM, Andy Lutomirski wrote:
>> ...
>>>> I could imagine that some kernel person would want to use even more
>>>> keys, but I think two fixed keys are kind of the minimal we'd want to
>>>> use.
>>>
>>> I imagine we'd reserve key 0 for normal page and key 1 for deny-read.
>>> Let me be a bit more concrete about what I'm suggesting:
>>>
>>> We'd have thread_struct.baseline_pkru.  It would start with key 0
>>> allowing all access and key 1 denying reads.
>>
>> Are you sure thread_struct is the right place for this?  I think of
>> signal handlers as a process-wide thing, and it seems a bit goofy if we
>> have the PKRU value in a signal handler depend on the PKRU of the thread
>> that got interrupted.
>
> I think you're right.  mmu_context_t might be a better choice.
>
>>
>>> We'd have a syscall like set_protection_key that could allocate unused
>>> keys and change the values of keys that have been allocated.  Those
>>> changes would be reflected in baseline_pkru.  Changes to keys 0 and 1
>>> in baseline_pkru would not be allowed.
>>
>> FWIW, I think we can do this without *actually* dedicating key 1 to
>> execute-only.  But that's a side issue.
>>
>>> Signal delivery would load baseline_pkru into the PKRU register.
>>> Signal restore would restore PKRU to its previous value.
>>
>> Do you really mean "its previous value" or are you OK with the existing
>> behavior which restores PKRU from the XSAVE buffer in the sigcontext?
>
> By "its previous value" I meant the value in the XSAVE buffer in the
> sigcontext.  So I think I'm okay with that :)
>
>>
>>> WRPKRU would, of course, override baseline_pkru, but it wouldn't
>>> change baseline_pkru.  The set_protection_key syscall would modify
>>> *both* real PKRU and baseline_pkru.
>>
>> How about this:
>>
>> We make baseline_pkru a process-wide baseline and store it in
>> mm->context.  That way, no matter which thread gets interrupted for a
>> signal, they see consistent values.  We only write to it when an app
>> _specifically_ asks for it to be updated with a special flag to
>> sys_pkey_set().
>>
>> When an app uses the execute-only support, we implicitly set the
>> read-disable bit in baseline_pkru for the execute-only pkey.
>
> Sounds good, I think.

Resurrecting an old thread, but:

Looking at your git tree, which I assume is a reasonably approximation
of your current patches, this seems to be unimplemented.  I, at least,
would be nervous about using PKRU for protection of critical data if
signal handlers are unconditionally exempt.

Also, the lazily allocated no-read key for execute-only is done in the
name of performance, but it results in odd semantics.  How much of a
performance win is preserving the init optimization of PKRU in
practice?  (I.e. how much faster are XSAVE and XRSTOR?)  I can't test
because even my Skylake laptop doesn't have PKRU.

--Andy