+static inline void pks_update_protection(int pkey, unsigned long protection) +{
- current->thread.saved_pkrs = update_pkey_val(current->thread.saved_pkrs,
pkey, protection);- preempt_disable();
- write_pkrs(current->thread.saved_pkrs);
- preempt_enable();
+}
Why does this need preempt count manipulation in addition to the get/put_cpu_var() inside of write_pkrs()?
+/**
- PKS access control functions
- Change the access of the domain specified by the pkey. These are global
- updates. They only affects the current running thread. It is undefined and
- a bug for users to call this without having allocated a pkey and using it as
- pkey here.
- pks_mknoaccess()
Disable all access to the domain
- pks_mkread()
Make the domain Read only
- pks_mkrdwr()
Make the domain Read/Write
- @pkey the pkey for which the access should change.
- */
+void pks_mknoaccess(int pkey) +{
- pks_update_protection(pkey, PKEY_DISABLE_ACCESS);
+} +EXPORT_SYMBOL_GPL(pks_mknoaccess);
These are named like PTE manipulation functions, which is kinda weird.
What's wrong with: pks_disable_access(pkey) ?
+void pks_mkread(int pkey) +{
- pks_update_protection(pkey, PKEY_DISABLE_WRITE);
+} +EXPORT_SYMBOL_GPL(pks_mkread);
I really don't like this name. It doesn't make readable, or even read-only, *especially* if it was already access-disabled.
+static const char pks_key_user0[] = "kernel";
+/* Store names of allocated keys for debug. Key 0 is reserved for the kernel. */ +static const char *pks_key_users[PKS_NUM_KEYS] = {
- pks_key_user0
+};
+/*
- Each key is represented by a bit. Bit 0 is set for key 0 and reserved for
- its use. We use ulong for the bit operations but only 16 bits are used.
- */
+static unsigned long pks_key_allocation_map = 1 << PKS_KERN_DEFAULT_KEY;
+/*
- pks_key_alloc - Allocate a PKS key
- @pkey_user: String stored for debugging of key exhaustion. The caller is
- responsible to maintain this memory until pks_key_free().
- */
+int pks_key_alloc(const char * const pkey_user) +{
- int nr;
- if (!cpu_feature_enabled(X86_FEATURE_PKS))
return -EINVAL;
I'm not sure I like -EINVAL for this. I thought we returned -ENOSPC for this case for user pkeys.
- while (1) {
nr = find_first_zero_bit(&pks_key_allocation_map, PKS_NUM_KEYS);if (nr >= PKS_NUM_KEYS) {pr_info("Cannot allocate supervisor key for %s.\n",pkey_user);return -ENOSPC;}if (!test_and_set_bit_lock(nr, &pks_key_allocation_map))break;- }
- /* for debugging key exhaustion */
- pks_key_users[nr] = pkey_user;
- return nr;
+} +EXPORT_SYMBOL_GPL(pks_key_alloc);
+/*
- pks_key_free - Free a previously allocate PKS key
- @pkey: Key to be free'ed
- */
+void pks_key_free(int pkey) +{
- if (!cpu_feature_enabled(X86_FEATURE_PKS))
return;- if (pkey >= PKS_NUM_KEYS || pkey <= PKS_KERN_DEFAULT_KEY)
return;
This seems worthy of a WARN_ON_ONCE() at least. It's essentially corrupt data coming into a kernel API.