[PATCH] Linux: Implement interfaces for memory protection keys

[Florian Weimer]

On 11/28/2017 08:01 PM, Adhemerval Zanella wrote:

>>> My understanding from documentation it key 0 is default one and it
>>> is assigned to any memory regions (through mmap) which has not been
>>> explicitly assigned through pkey_mprotect. My questioning whether we
>>> should filter out key 0 is to:
>>>
>>>  Â Â  1. Issue an error when application tries to use a key which has
>>>  Â Â Â Â Â  not been previously allocated by pkey_alloc (since key 0 is
>>>  Â Â Â Â Â  pre-allocated by the kernel).
>>>
>>>  Â Â  2. Avoid changing default masking bits for pages which has not
>>>  Â Â Â Â Â  been explicitly assigned through a pkey_mprotect.
>>>
>>> However this does not prevent uses from issuing a RDPKRU/WRPKRU
>>> directly.
>>
>> There is no way to probe allocated keys, so I don't think we can implement more extensive checking without further kernel interfaces.
> 
> That was my understanding as well, I was just checking if I missed something
> reading pkey documentation (from both architecture and kernel level). I
> noticed there is some idea of extending the sysfs to provide more information
> about the system supported keys [1], but even through it also does not
> really provide the information of the allocated keys.
> 
> [1] https://marc.info/?l=linux-kernel&m=150995950219669&w=2

I've since learned that the PROT_EXEC key is dynamically allocated, too. 
  It's not always 1.  So this means that we can't consistently reject 
keys used by the kernel.

>> +* Support for memory protection keys was added.  The <sys/mman.h> header now
>> +  declares the functions pkey_alloc, pkey_free, pkey_memprotect, pkey_set,
>> +  pkey_get.
>> +
> > Typo here (s/memprotect/mprotect).

Thanks, fixed.

>> +New threads and subprocesses inherit the access rights of the current
>> +thread.  If a protection key is allocated subsequently, existing threads
>> +(except the current) will use an unspecified system default for the
>> +access rights associated with newly allocated keys.
> 
> For subprocesses isn't the case that the default key 0 is used instead,
> instead of the 'unspecified' one?

Only after execve, I hope.  It would be weird if PROT_EXEC memory turned 
readable in the subprocess after fork.  It would also totally break my 
idea to use this for write-protecting the GOT most of the time. 8-P

I just verified this on actual hardware.  Should I add another test for 
that, maybe tst-pkey-fork?  Maybe as a separate patch?

>> +@deftypefun int pkey_free (int @var{key})
>> +@standards{Linux, sys/mman.h}
>> +@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
>> +Deallocate the protection key, so that it can be reused by
>> +@code{pkey_alloc}.
>> +
>> +Calling this function does not change the access rights of the freed
>> +protection key.  The calling thread and other threads may retain access
>> +to it, even if it is subsequently allocated again.  For this reason, it
>> +is not recommended to call the @code{pkey_free} function.
> 
> I presume you are referring to your 'MPK: pkey_free and key reuse' discussion,
> but should we add a note that 'pkey_free' should be as long as the application
> make sure to munmap the pages associated with the key meant to free first?

No, it's also relevant that threads retain access when the key is reused.

>> +Some systems use memory protection keys to emulate certain combinations
>> +of @var{protection} flags.  Under such circumstances, specifying an
>> +explicit protection key may behave as if additional flags have been
>> +specified in @var{protection}, even though this does not happen with the
>> +default protection key.  For example, some systems can supported
> 
> Shouldn't it be 'can support'?

Yes, fixed.

>> +int
>> +pkey_mprotect (void *addr, size_t len, int prot, int pkey)
>> +{
>> +  if (pkey == -1)
>> +    /* If the key is -1, the system call is precisely equivalent to
>> +       mprotect.  */
>> +    return __mprotect (addr, len, prot);
>> +#ifdef __NR_pkey_mprotect
>> +  return INLINE_SYSCALL (pkey_mprotect, 4, addr, len, prot, pkey);
>> +#else
>> +  __set_errno (ENOSYS);
>> +  return -1;
>> +#endif
>> +}
> 
> I would suggest to use INLINE_SYSCALL_CALL here to omit the argument number.

Right, I made the change.


>> +  keys[0] = pkey_alloc (0, 0);
>> +  if (keys[0] < 0)
>> +    {
>> +      if (errno == ENOSYS)
>> +        {
>> +          puts ("warning: kernel does not support memory protection keys");
>> +          return EXIT_UNSUPPORTED;
>> +        }
> 
> We have FAIL_UNSUPPORTED for these cases.

Okay, I switched to that.

>> +      /* The manual page says that ENOSPC is used to report lack of
>> +         CPU support, but the kernel returns EINVAL.  */
>> +      if (errno == ENOSPC || errno == EINVAL)
>> +        {
>> +          printf ("warning: CPU does not support memory protection keys: %m");
>> +          return EXIT_UNSUPPORTED;
>> +        }
>> +      FAIL_EXIT1 ("pkey_alloc: %m");
> 
> I do not think we should comment the manpages description because it could
> changed.

Fair enough, I dropped the commend and the check for ENOSPC.

The attached patch moves the implementation to the x86 directory because 
it turned out the syscalls work on i386 as well (but presumably only 
with a 64-bit kernel).

Thanks,
Florian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mpk.patch
Type: text/x-patch
Size: 61983 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20171201/cc81dbeb/attachment.bin>