diff mbox series

[RFC,v2,04/13] x86/mm: Add helper functions for MKTME memory encryption keys

Message ID bd83f72d30ccfc7c1bc7ce9ab81bdf66e78a1d7d.1543903910.git.alison.schofield@intel.com (mailing list archive)
State New, archived
Headers show
Series Multi-Key Total Memory Encryption API (MKTME) | expand

Commit Message

Alison Schofield Dec. 4, 2018, 7:39 a.m. UTC 
Define a global mapping structure to manage the mapping of userspace
Keys to hardware KeyIDs in MKTME (Multi-Key Total Memory Encryption).
Implement helper functions that access this mapping structure.

The helpers will be used by these MKTME API's:
 >  Key Service API: security/keys/mktme_keys.c
 >  encrypt_mprotect() system call: mm/mprotect.c

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 arch/x86/include/asm/mktme.h | 12 ++++++
 arch/x86/mm/mktme.c          | 91 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 103 insertions(+)

Comments

Peter Zijlstra Dec. 4, 2018, 9:14 a.m. UTC | #1 
On Mon, Dec 03, 2018 at 11:39:51PM -0800, Alison Schofield wrote:
> +int mktme_map_keyid_from_key(void *key)
> +{
> +	int i;
> +
> +	for (i = 1; i <= mktme_nr_keyids; i++)
> +		if (mktme_map->key[i] == key)
> +			return i;

CodingStyle

> +	return 0;
> +}
> +int mktme_map_get_free_keyid(void)
> +{
> +	int i;
> +
> +	if (mktme_map->mapped_keyids < mktme_nr_keyids) {
> +		for (i = 1; i <= mktme_nr_keyids; i++)
> +			if (mktme_map->key[i] == 0)
> +				return i;

CodingStyle

> +	}
> +	return 0;
> +}
Andy Lutomirski Dec. 4, 2018, 3:35 p.m. UTC | #2 
> On Dec 3, 2018, at 11:39 PM, Alison Schofield <alison.schofield@intel.com> wrote:
> 
> Define a global mapping structure to manage the mapping of userspace
> Keys to hardware KeyIDs in MKTME (Multi-Key Total Memory Encryption).
> Implement helper functions that access this mapping structure.
> 

Why is a key “void *”?  Who owns the memory?  Can a real type be used?
Alison Schofield Dec. 5, 2018, 5:49 a.m. UTC | #3 
On Tue, Dec 04, 2018 at 10:14:34AM +0100, Peter Zijlstra wrote:
> On Mon, Dec 03, 2018 at 11:39:51PM -0800, Alison Schofield wrote:
> 
> CodingStyle
> CodingStyle
>
Thanks Peter. I'll repair all the badly nested if statements.
Alison Schofield Dec. 5, 2018, 5:52 a.m. UTC | #4 
On Tue, Dec 04, 2018 at 07:35:50AM -0800, Andy Lutomirski wrote:
> 
> 
> > On Dec 3, 2018, at 11:39 PM, Alison Schofield <alison.schofield@intel.com> wrote:
> > 
> > Define a global mapping structure to manage the mapping of userspace
> > Keys to hardware KeyIDs in MKTME (Multi-Key Total Memory Encryption).
> > Implement helper functions that access this mapping structure.
> > 
> 
> Why is a key “void *”?  Who owns the memory?  Can a real type be used?
>
It's of type "struct key" of the kernel key service.
Replacing void w 'struct key'.
Jarkko Sakkinen Dec. 6, 2018, 8:31 a.m. UTC | #5 
On Mon, 2018-12-03 at 23:39 -0800, Alison Schofield wrote:
> +extern int mktme_map_alloc(void);
> +extern void mktme_map_free(void);
> +extern void mktme_map_lock(void);
> +extern void mktme_map_unlock(void);
> +extern int mktme_map_mapped_keyids(void);
> +extern void mktme_map_set_keyid(int keyid, void *key);
> +extern void mktme_map_free_keyid(int keyid);
> +extern int mktme_map_keyid_from_key(void *key);
> +extern void *mktme_map_key_from_keyid(int keyid);
> +extern int mktme_map_get_free_keyid(void);

No need for extern keyword for function declarations. It is
only needed for variable declarations.

> +
>  DECLARE_STATIC_KEY_FALSE(mktme_enabled_key);
>  static inline bool mktme_enabled(void)
>  {
> diff --git a/arch/x86/mm/mktme.c b/arch/x86/mm/mktme.c
> index c81727540e7c..34224d4e3f45 100644
> --- a/arch/x86/mm/mktme.c
> +++ b/arch/x86/mm/mktme.c
> @@ -40,6 +40,97 @@ int __vma_keyid(struct vm_area_struct *vma)
>  	return (prot & mktme_keyid_mask) >> mktme_keyid_shift;
>  }
>  
> +/*
> + * struct mktme_map and the mktme_map_* functions manage the mapping
> + * of userspace Keys to hardware KeyIDs. These are used by the MKTME Key

What are "userspace Keys" anyway and why Key and not key?

> + * Service API and the encrypt_mprotect() system call.
> + */
> +
> +struct mktme_mapping {
> +	struct mutex	lock;		/* protect this map & HW state */
> +	unsigned int	mapped_keyids;
> +	void		*key[];
> +};

Personally, I prefer not to align struct fields (I do align enums
because there it makes more sense) as often you end up realigning
everything.

Documentation would bring more clarity. For example, what does key[]
contain, why there is a lock and what mapped_keyids field contains?

/Jarkko
diff mbox series

Patch

diff --git a/arch/x86/include/asm/mktme.h b/arch/x86/include/asm/mktme.h
index f05baa15e6f6..dbb49909d665 100644
--- a/arch/x86/include/asm/mktme.h
+++ b/arch/x86/include/asm/mktme.h
@@ -12,6 +12,18 @@  extern phys_addr_t mktme_keyid_mask;
 extern int mktme_nr_keyids;
 extern int mktme_keyid_shift;
 
+/* Manage mappings between hardware KeyIDs and userspace Keys */
+extern int mktme_map_alloc(void);
+extern void mktme_map_free(void);
+extern void mktme_map_lock(void);
+extern void mktme_map_unlock(void);
+extern int mktme_map_mapped_keyids(void);
+extern void mktme_map_set_keyid(int keyid, void *key);
+extern void mktme_map_free_keyid(int keyid);
+extern int mktme_map_keyid_from_key(void *key);
+extern void *mktme_map_key_from_keyid(int keyid);
+extern int mktme_map_get_free_keyid(void);
+
 DECLARE_STATIC_KEY_FALSE(mktme_enabled_key);
 static inline bool mktme_enabled(void)
 {
diff --git a/arch/x86/mm/mktme.c b/arch/x86/mm/mktme.c
index c81727540e7c..34224d4e3f45 100644
--- a/arch/x86/mm/mktme.c
+++ b/arch/x86/mm/mktme.c
@@ -40,6 +40,97 @@  int __vma_keyid(struct vm_area_struct *vma)
 	return (prot & mktme_keyid_mask) >> mktme_keyid_shift;
 }
 
+/*
+ * struct mktme_map and the mktme_map_* functions manage the mapping
+ * of userspace Keys to hardware KeyIDs. These are used by the MKTME Key
+ * Service API and the encrypt_mprotect() system call.
+ */
+
+struct mktme_mapping {
+	struct mutex	lock;		/* protect this map & HW state */
+	unsigned int	mapped_keyids;
+	void		*key[];
+};
+
+struct mktme_mapping *mktme_map;
+
+static inline long mktme_map_size(void)
+{
+	long size = 0;
+
+	size += sizeof(*mktme_map);
+	size += sizeof(mktme_map->key[0]) * (mktme_nr_keyids + 1);
+	return size;
+}
+
+int mktme_map_alloc(void)
+{
+	mktme_map = kvzalloc(mktme_map_size(), GFP_KERNEL);
+	if (!mktme_map)
+		return 0;
+	mutex_init(&mktme_map->lock);
+	return 1;
+}
+
+void mktme_map_free(void)
+{
+	kvfree(mktme_map);
+}
+
+void mktme_map_lock(void)
+{
+	mutex_lock(&mktme_map->lock);
+}
+
+void mktme_map_unlock(void)
+{
+	mutex_unlock(&mktme_map->lock);
+}
+
+int mktme_map_mapped_keyids(void)
+{
+	return mktme_map->mapped_keyids;
+}
+
+void mktme_map_set_keyid(int keyid, void *key)
+{
+	mktme_map->key[keyid] = key;
+	mktme_map->mapped_keyids++;
+}
+
+void mktme_map_free_keyid(int keyid)
+{
+	mktme_map->key[keyid] = 0;
+	mktme_map->mapped_keyids--;
+}
+
+int mktme_map_keyid_from_key(void *key)
+{
+	int i;
+
+	for (i = 1; i <= mktme_nr_keyids; i++)
+		if (mktme_map->key[i] == key)
+			return i;
+	return 0;
+}
+
+void *mktme_map_key_from_keyid(int keyid)
+{
+	return mktme_map->key[keyid];
+}
+
+int mktme_map_get_free_keyid(void)
+{
+	int i;
+
+	if (mktme_map->mapped_keyids < mktme_nr_keyids) {
+		for (i = 1; i <= mktme_nr_keyids; i++)
+			if (mktme_map->key[i] == 0)
+				return i;
+	}
+	return 0;
+}
+
 /* Prepare page to be used for encryption. Called from page allocator. */
 void __prep_encrypted_page(struct page *page, int order, int keyid, bool zero)
 {