[PATCH 0/7] keys: Miscellany

[PATCH 0/7] keys: Miscellany

[David Howells]

Here are some miscellaneous keyrings fixes and improvements intended for
the next merge window:

 (1) Fix a bunch of warnings from sparse, including missing RCU bits and
     kdoc-function argument mismatches

 (2) Implement a keyctl to allow a key to be moved from one keyring to
     another, with the option of prohibiting key replacement in the
     destination keyring.

 (3) Grant Link permission to possessors of request_key_auth tokens so that
     upcall servicing daemons can more easily arrange things such that only
     the necessary auth key is passed to the actual service program, and
     not all the auth keys a daemon might possesss.

The patches can be found on the following branch:

	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-misc

David
---
David Howells (7):
      keys: sparse: Fix key_fs[ug]id_changed()
      keys: sparse: Fix incorrect RCU accesses
      keys: sparse: Fix kdoc mismatches
      keys: Break bits out of key_unlink()
      keys: Make __key_link_begin() handle lockdep nesting
      keys: Add a keyctl to move a key between keyrings
      keys: Grant Link permission to possessers of request_key auth keys


 include/linux/key.h              |   13 ++-
 include/uapi/linux/keyctl.h      |    3 +
 kernel/cred.c                    |    4 -
 security/keys/compat.c           |    3 +
 security/keys/internal.h         |    3 -
 security/keys/key.c              |    6 +
 security/keys/keyctl.c           |   58 ++++++++++++
 security/keys/keyring.c          |  178 ++++++++++++++++++++++++++++++++++----
 security/keys/process_keys.c     |   22 ++---
 security/keys/request_key.c      |    4 -
 security/keys/request_key_auth.c |    4 -
 11 files changed, 250 insertions(+), 48 deletions(-)

[PATCH 0/7] keys: Miscellany

[David Howells]

Here are some miscellaneous keyrings fixes and improvements intended for
the next merge window:

 (1) Fix a bunch of warnings from sparse, including missing RCU bits and
     kdoc-function argument mismatches

 (2) Implement a keyctl to allow a key to be moved from one keyring to
     another, with the option of prohibiting key replacement in the
     destination keyring.

 (3) Grant Link permission to possessors of request_key_auth tokens so that
     upcall servicing daemons can more easily arrange things such that only
     the necessary auth key is passed to the actual service program, and
     not all the auth keys a daemon might possesss.

The patches can be found on the following branch:

	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-misc

David
---
David Howells (7):
      keys: sparse: Fix key_fs[ug]id_changed()
      keys: sparse: Fix incorrect RCU accesses
      keys: sparse: Fix kdoc mismatches
      keys: Break bits out of key_unlink()
      keys: Make __key_link_begin() handle lockdep nesting
      keys: Add a keyctl to move a key between keyrings
      keys: Grant Link permission to possessers of request_key auth keys


 include/linux/key.h              |   13 ++-
 include/uapi/linux/keyctl.h      |    3 +
 kernel/cred.c                    |    4 -
 security/keys/compat.c           |    3 +
 security/keys/internal.h         |    3 -
 security/keys/key.c              |    6 +
 security/keys/keyctl.c           |   58 ++++++++++++
 security/keys/keyring.c          |  178 ++++++++++++++++++++++++++++++++++----
 security/keys/process_keys.c     |   22 ++---
 security/keys/request_key.c      |    4 -
 security/keys/request_key_auth.c |    4 -
 11 files changed, 250 insertions(+), 48 deletions(-)

[PATCH 1/7] keys: sparse: Fix key_fs[ug]id_changed()

[David Howells]

Sparse warnings are incurred by key_fs[ug]id_changed() due to unprotected
accesses of tsk->cred, which is marked __rcu.

Fix this by passing the new cred struct to these functions from
commit_creds() rather than the task pointer.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 include/linux/key.h          |    8 ++++----
 kernel/cred.c                |    4 ++--
 security/keys/process_keys.c |   22 ++++++++++------------
 3 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index 7099985e35a9..1f09aad1c98c 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -402,8 +402,8 @@ extern struct ctl_table key_sysctls[];
  * the userspace interface
  */
 extern int install_thread_keyring_to_cred(struct cred *cred);
-extern void key_fsuid_changed(struct task_struct *tsk);
-extern void key_fsgid_changed(struct task_struct *tsk);
+extern void key_fsuid_changed(struct cred *new_cred);
+extern void key_fsgid_changed(struct cred *new_cred);
 extern void key_init(void);
 
 #else /* CONFIG_KEYS */
@@ -418,8 +418,8 @@ extern void key_init(void);
 #define make_key_ref(k, p)		NULL
 #define key_ref_to_ptr(k)		NULL
 #define is_key_possessed(k)		0
-#define key_fsuid_changed(t)		do { } while(0)
-#define key_fsgid_changed(t)		do { } while(0)
+#define key_fsuid_changed(c)		do { } while(0)
+#define key_fsgid_changed(c)		do { } while(0)
 #define key_init()			do { } while(0)
 
 #endif /* CONFIG_KEYS */
diff --git a/kernel/cred.c b/kernel/cred.c
index 45d77284aed0..3bd40de9e192 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -455,9 +455,9 @@ int commit_creds(struct cred *new)
 
 	/* alter the thread keyring */
 	if (!uid_eq(new->fsuid, old->fsuid))
-		key_fsuid_changed(task);
+		key_fsuid_changed(new);
 	if (!gid_eq(new->fsgid, old->fsgid))
-		key_fsgid_changed(task);
+		key_fsgid_changed(new);
 
 	/* do it
 	 * RLIMIT_NPROC limits on user->processes have already been checked
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index f05f7125a7d5..ba5d3172cafe 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -293,28 +293,26 @@ static int install_session_keyring(struct key *keyring)
 /*
  * Handle the fsuid changing.
  */
-void key_fsuid_changed(struct task_struct *tsk)
+void key_fsuid_changed(struct cred *new_cred)
 {
 	/* update the ownership of the thread keyring */
-	BUG_ON(!tsk->cred);
-	if (tsk->cred->thread_keyring) {
-		down_write(&tsk->cred->thread_keyring->sem);
-		tsk->cred->thread_keyring->uid = tsk->cred->fsuid;
-		up_write(&tsk->cred->thread_keyring->sem);
+	if (new_cred->thread_keyring) {
+		down_write(&new_cred->thread_keyring->sem);
+		new_cred->thread_keyring->uid = new_cred->fsuid;
+		up_write(&new_cred->thread_keyring->sem);
 	}
 }
 
 /*
  * Handle the fsgid changing.
  */
-void key_fsgid_changed(struct task_struct *tsk)
+void key_fsgid_changed(struct cred *new_cred)
 {
 	/* update the ownership of the thread keyring */
-	BUG_ON(!tsk->cred);
-	if (tsk->cred->thread_keyring) {
-		down_write(&tsk->cred->thread_keyring->sem);
-		tsk->cred->thread_keyring->gid = tsk->cred->fsgid;
-		up_write(&tsk->cred->thread_keyring->sem);
+	if (new_cred->thread_keyring) {
+		down_write(&new_cred->thread_keyring->sem);
+		new_cred->thread_keyring->gid = new_cred->fsgid;
+		up_write(&new_cred->thread_keyring->sem);
 	}
 }
 

[PATCH 1/7] keys: sparse: Fix key_fs[ug]id_changed()

[David Howells]

Sparse warnings are incurred by key_fs[ug]id_changed() due to unprotected
accesses of tsk->cred, which is marked __rcu.

Fix this by passing the new cred struct to these functions from
commit_creds() rather than the task pointer.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 include/linux/key.h          |    8 ++++----
 kernel/cred.c                |    4 ++--
 security/keys/process_keys.c |   22 ++++++++++------------
 3 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index 7099985e35a9..1f09aad1c98c 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -402,8 +402,8 @@ extern struct ctl_table key_sysctls[];
  * the userspace interface
  */
 extern int install_thread_keyring_to_cred(struct cred *cred);
-extern void key_fsuid_changed(struct task_struct *tsk);
-extern void key_fsgid_changed(struct task_struct *tsk);
+extern void key_fsuid_changed(struct cred *new_cred);
+extern void key_fsgid_changed(struct cred *new_cred);
 extern void key_init(void);
 
 #else /* CONFIG_KEYS */
@@ -418,8 +418,8 @@ extern void key_init(void);
 #define make_key_ref(k, p)		NULL
 #define key_ref_to_ptr(k)		NULL
 #define is_key_possessed(k)		0
-#define key_fsuid_changed(t)		do { } while(0)
-#define key_fsgid_changed(t)		do { } while(0)
+#define key_fsuid_changed(c)		do { } while(0)
+#define key_fsgid_changed(c)		do { } while(0)
 #define key_init()			do { } while(0)
 
 #endif /* CONFIG_KEYS */
diff --git a/kernel/cred.c b/kernel/cred.c
index 45d77284aed0..3bd40de9e192 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -455,9 +455,9 @@ int commit_creds(struct cred *new)
 
 	/* alter the thread keyring */
 	if (!uid_eq(new->fsuid, old->fsuid))
-		key_fsuid_changed(task);
+		key_fsuid_changed(new);
 	if (!gid_eq(new->fsgid, old->fsgid))
-		key_fsgid_changed(task);
+		key_fsgid_changed(new);
 
 	/* do it
 	 * RLIMIT_NPROC limits on user->processes have already been checked
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index f05f7125a7d5..ba5d3172cafe 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -293,28 +293,26 @@ static int install_session_keyring(struct key *keyring)
 /*
  * Handle the fsuid changing.
  */
-void key_fsuid_changed(struct task_struct *tsk)
+void key_fsuid_changed(struct cred *new_cred)
 {
 	/* update the ownership of the thread keyring */
-	BUG_ON(!tsk->cred);
-	if (tsk->cred->thread_keyring) {
-		down_write(&tsk->cred->thread_keyring->sem);
-		tsk->cred->thread_keyring->uid = tsk->cred->fsuid;
-		up_write(&tsk->cred->thread_keyring->sem);
+	if (new_cred->thread_keyring) {
+		down_write(&new_cred->thread_keyring->sem);
+		new_cred->thread_keyring->uid = new_cred->fsuid;
+		up_write(&new_cred->thread_keyring->sem);
 	}
 }
 
 /*
  * Handle the fsgid changing.
  */
-void key_fsgid_changed(struct task_struct *tsk)
+void key_fsgid_changed(struct cred *new_cred)
 {
 	/* update the ownership of the thread keyring */
-	BUG_ON(!tsk->cred);
-	if (tsk->cred->thread_keyring) {
-		down_write(&tsk->cred->thread_keyring->sem);
-		tsk->cred->thread_keyring->gid = tsk->cred->fsgid;
-		up_write(&tsk->cred->thread_keyring->sem);
+	if (new_cred->thread_keyring) {
+		down_write(&new_cred->thread_keyring->sem);
+		new_cred->thread_keyring->gid = new_cred->fsgid;
+		up_write(&new_cred->thread_keyring->sem);
 	}
 }
 

Re: [PATCH 1/7] keys: sparse: Fix key_fs[ug]id_changed()

[James Morris]

On Wed, 22 May 2019, David Howells wrote:

> Sparse warnings are incurred by key_fs[ug]id_changed() due to unprotected
> accesses of tsk->cred, which is marked __rcu.
> 
> Fix this by passing the new cred struct to these functions from
> commit_creds() rather than the task pointer.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>


Reviewed-by: James Morris <jamorris@linux.microsoft.com>


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 1/7] keys: sparse: Fix key_fs[ug]id_changed()

[James Morris]

On Wed, 22 May 2019, David Howells wrote:

> Sparse warnings are incurred by key_fs[ug]id_changed() due to unprotected
> accesses of tsk->cred, which is marked __rcu.
> 
> Fix this by passing the new cred struct to these functions from
> commit_creds() rather than the task pointer.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>


Reviewed-by: James Morris <jamorris@linux.microsoft.com>


-- 
James Morris
<jmorris@namei.org>

[PATCH 2/7] keys: sparse: Fix incorrect RCU accesses

[David Howells]

Fix a pair of accesses that should be using RCU protection.

rcu_dereference_protected() is needed to access task_struct::real_parent.

current_cred() should be used to access current->cred.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/keys/keyctl.c           |    3 ++-
 security/keys/request_key_auth.c |    2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 3e4053a217c3..0f947bcbad46 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1524,7 +1524,8 @@ long keyctl_session_to_parent(void)
 
 	ret = -EPERM;
 	oldwork = NULL;
-	parent = me->real_parent;
+	parent = rcu_dereference_protected(me->real_parent,
+					   lockdep_is_held(&tasklist_lock));
 
 	/* the parent mustn't be init and mustn't be a kernel thread */
 	if (parent->pid <= 1 || !parent->mm)
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index bda6201c6c45..572c7a60473a 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -152,7 +152,7 @@ struct key *request_key_auth_new(struct key *target, const char *op,
 				 struct key *dest_keyring)
 {
 	struct request_key_auth *rka, *irka;
-	const struct cred *cred = current->cred;
+	const struct cred *cred = current_cred();
 	struct key *authkey = NULL;
 	char desc[20];
 	int ret = -ENOMEM;

[PATCH 2/7] keys: sparse: Fix incorrect RCU accesses

[David Howells]

Fix a pair of accesses that should be using RCU protection.

rcu_dereference_protected() is needed to access task_struct::real_parent.

current_cred() should be used to access current->cred.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/keys/keyctl.c           |    3 ++-
 security/keys/request_key_auth.c |    2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 3e4053a217c3..0f947bcbad46 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1524,7 +1524,8 @@ long keyctl_session_to_parent(void)
 
 	ret = -EPERM;
 	oldwork = NULL;
-	parent = me->real_parent;
+	parent = rcu_dereference_protected(me->real_parent,
+					   lockdep_is_held(&tasklist_lock));
 
 	/* the parent mustn't be init and mustn't be a kernel thread */
 	if (parent->pid <= 1 || !parent->mm)
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index bda6201c6c45..572c7a60473a 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -152,7 +152,7 @@ struct key *request_key_auth_new(struct key *target, const char *op,
 				 struct key *dest_keyring)
 {
 	struct request_key_auth *rka, *irka;
-	const struct cred *cred = current->cred;
+	const struct cred *cred = current_cred();
 	struct key *authkey = NULL;
 	char desc[20];
 	int ret = -ENOMEM;

Re: [PATCH 2/7] keys: sparse: Fix incorrect RCU accesses

[James Morris]

On Wed, 22 May 2019, David Howells wrote:

> Fix a pair of accesses that should be using RCU protection.
> 
> rcu_dereference_protected() is needed to access task_struct::real_parent.
> 
> current_cred() should be used to access current->cred.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>


Reviewed-by: James Morris <jamorris@linux.microsoft.com>


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 2/7] keys: sparse: Fix incorrect RCU accesses

[James Morris]

On Wed, 22 May 2019, David Howells wrote:

> Fix a pair of accesses that should be using RCU protection.
> 
> rcu_dereference_protected() is needed to access task_struct::real_parent.
> 
> current_cred() should be used to access current->cred.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>


Reviewed-by: James Morris <jamorris@linux.microsoft.com>


-- 
James Morris
<jmorris@namei.org>

[PATCH 3/7] keys: sparse: Fix kdoc mismatches

[David Howells]

Fix some kdoc argument description mismatches reported by sparse and give
keyring_restrict() a description.

Signed-off-by: David Howells <dhowells@redhat.com>
cc: Mat Martineau <mathew.j.martineau@linux.intel.com>
---

 security/keys/keyring.c     |   10 +++++++---
 security/keys/request_key.c |    2 +-
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index e14f09e3a4b0..5b218b270598 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -520,7 +520,7 @@ EXPORT_SYMBOL(keyring_alloc);
  * @keyring: The keyring being added to.
  * @type: The type of key being added.
  * @payload: The payload of the key intended to be added.
- * @data: Additional data for evaluating restriction.
+ * @restriction_key: Keys providing additional data for evaluating restriction.
  *
  * Reject the addition of any links to a keyring.  It can be overridden by
  * passing KEY_ALLOC_BYPASS_RESTRICTION to key_instantiate_and_link() when
@@ -976,9 +976,13 @@ static bool keyring_detect_restriction_cycle(const struct key *dest_keyring,
 
 /**
  * keyring_restrict - Look up and apply a restriction to a keyring
- *
- * @keyring: The keyring to be restricted
+ * @keyring_ref: The keyring to be restricted
+ * @type: The key type that will provide the restriction checker.
  * @restriction: The restriction options to apply to the keyring
+ *
+ * Look up a keyring and apply a restriction to it.  The restriction is managed
+ * by the specific key type, but can be configured by the options specified in
+ * the restriction string.
  */
 int keyring_restrict(key_ref_t keyring_ref, const char *type,
 		     const char *restriction)
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 75d87f9e0f49..1f234b019437 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -24,7 +24,7 @@
 
 /**
  * complete_request_key - Complete the construction of a key.
- * @auth_key: The authorisation key.
+ * @authkey: The authorisation key.
  * @error: The success or failute of the construction.
  *
  * Complete the attempt to construct a key.  The key will be negated

[PATCH 3/7] keys: sparse: Fix kdoc mismatches

[David Howells]

Fix some kdoc argument description mismatches reported by sparse and give
keyring_restrict() a description.

Signed-off-by: David Howells <dhowells@redhat.com>
cc: Mat Martineau <mathew.j.martineau@linux.intel.com>
---

 security/keys/keyring.c     |   10 +++++++---
 security/keys/request_key.c |    2 +-
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index e14f09e3a4b0..5b218b270598 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -520,7 +520,7 @@ EXPORT_SYMBOL(keyring_alloc);
  * @keyring: The keyring being added to.
  * @type: The type of key being added.
  * @payload: The payload of the key intended to be added.
- * @data: Additional data for evaluating restriction.
+ * @restriction_key: Keys providing additional data for evaluating restriction.
  *
  * Reject the addition of any links to a keyring.  It can be overridden by
  * passing KEY_ALLOC_BYPASS_RESTRICTION to key_instantiate_and_link() when
@@ -976,9 +976,13 @@ static bool keyring_detect_restriction_cycle(const struct key *dest_keyring,
 
 /**
  * keyring_restrict - Look up and apply a restriction to a keyring
- *
- * @keyring: The keyring to be restricted
+ * @keyring_ref: The keyring to be restricted
+ * @type: The key type that will provide the restriction checker.
  * @restriction: The restriction options to apply to the keyring
+ *
+ * Look up a keyring and apply a restriction to it.  The restriction is managed
+ * by the specific key type, but can be configured by the options specified in
+ * the restriction string.
  */
 int keyring_restrict(key_ref_t keyring_ref, const char *type,
 		     const char *restriction)
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 75d87f9e0f49..1f234b019437 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -24,7 +24,7 @@
 
 /**
  * complete_request_key - Complete the construction of a key.
- * @auth_key: The authorisation key.
+ * @authkey: The authorisation key.
  * @error: The success or failute of the construction.
  *
  * Complete the attempt to construct a key.  The key will be negated

Re: [PATCH 3/7] keys: sparse: Fix kdoc mismatches

[James Morris]

On Wed, 22 May 2019, David Howells wrote:

> Fix some kdoc argument description mismatches reported by sparse and give
> keyring_restrict() a description.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Mat Martineau <mathew.j.martineau@linux.intel.com>


Reviewed-by: James Morris <jamorris@linux.microsoft.com>


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 3/7] keys: sparse: Fix kdoc mismatches

[James Morris]

On Wed, 22 May 2019, David Howells wrote:

> Fix some kdoc argument description mismatches reported by sparse and give
> keyring_restrict() a description.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Mat Martineau <mathew.j.martineau@linux.intel.com>


Reviewed-by: James Morris <jamorris@linux.microsoft.com>


-- 
James Morris
<jmorris@namei.org>

[PATCH 4/7] keys: Break bits out of key_unlink()

[David Howells]

Break bits out of key_unlink() into helper functions so that they can be
used in implementing key_move().

Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/keys/keyring.c |   86 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 65 insertions(+), 21 deletions(-)

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 5b218b270598..2cfeeeaa1ffa 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1382,6 +1382,65 @@ int key_link(struct key *keyring, struct key *key)
 }
 EXPORT_SYMBOL(key_link);
 
+/*
+ * Begin the process of unlinking a key from a keyring.
+ */
+static int __key_unlink_begin(struct key *keyring, unsigned int lock_nesting,
+			      struct key *key, struct assoc_array_edit **_edit)
+	__acquires(&keyring->sem)
+{
+	struct assoc_array_edit *edit;
+	int ret;
+
+	if (keyring->type != &key_type_keyring)
+		return -ENOTDIR;
+
+	down_write_nested(&keyring->sem, lock_nesting);
+
+	edit = assoc_array_delete(&keyring->keys, &keyring_assoc_array_ops,
+				  &key->index_key);
+	if (IS_ERR(edit)) {
+		ret = PTR_ERR(edit);
+		goto error;
+	}
+
+	if (!edit) {
+		ret = -ENOENT;
+		goto error;
+	}
+
+	*_edit = edit;
+	return 0;
+
+error:
+	up_write(&keyring->sem);
+	return ret;
+}
+
+/*
+ * Apply an unlink change.
+ */
+static void __key_unlink(struct key *keyring, struct key *key,
+			      struct assoc_array_edit **_edit)
+{
+	assoc_array_apply_edit(*_edit);
+	*_edit = NULL;
+	key_payload_reserve(keyring, keyring->datalen - KEYQUOTA_LINK_BYTES);
+}
+
+/*
+ * Finish unlinking a key from to a keyring.
+ */
+static void __key_unlink_end(struct key *keyring,
+			     struct key *key,
+			     struct assoc_array_edit *edit)
+	__releases(&keyring->sem)
+{
+	if (edit)
+		assoc_array_cancel_edit(edit);
+	up_write(&keyring->sem);
+}
+
 /**
  * key_unlink - Unlink the first link to a key from a keyring.
  * @keyring: The keyring to remove the link from.
@@ -1407,28 +1466,13 @@ int key_unlink(struct key *keyring, struct key *key)
 	key_check(keyring);
 	key_check(key);
 
-	if (keyring->type != &key_type_keyring)
-		return -ENOTDIR;
-
-	down_write(&keyring->sem);
-
-	edit = assoc_array_delete(&keyring->keys, &keyring_assoc_array_ops,
-				  &key->index_key);
-	if (IS_ERR(edit)) {
-		ret = PTR_ERR(edit);
-		goto error;
-	}
-	ret = -ENOENT;
-	if (edit = NULL)
-		goto error;
-
-	assoc_array_apply_edit(edit);
-	key_payload_reserve(keyring, keyring->datalen - KEYQUOTA_LINK_BYTES);
-	ret = 0;
+	ret = __key_unlink_begin(keyring, 0, key, &edit);
+	if (ret < 0)
+		return ret;
 
-error:
-	up_write(&keyring->sem);
-	return ret;
+	__key_unlink(keyring, key, &edit);
+	__key_unlink_end(keyring, key, edit);
+	return 0;
 }
 EXPORT_SYMBOL(key_unlink);
 

[PATCH 4/7] keys: Break bits out of key_unlink()

[David Howells]

Break bits out of key_unlink() into helper functions so that they can be
used in implementing key_move().

Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/keys/keyring.c |   86 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 65 insertions(+), 21 deletions(-)

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 5b218b270598..2cfeeeaa1ffa 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1382,6 +1382,65 @@ int key_link(struct key *keyring, struct key *key)
 }
 EXPORT_SYMBOL(key_link);
 
+/*
+ * Begin the process of unlinking a key from a keyring.
+ */
+static int __key_unlink_begin(struct key *keyring, unsigned int lock_nesting,
+			      struct key *key, struct assoc_array_edit **_edit)
+	__acquires(&keyring->sem)
+{
+	struct assoc_array_edit *edit;
+	int ret;
+
+	if (keyring->type != &key_type_keyring)
+		return -ENOTDIR;
+
+	down_write_nested(&keyring->sem, lock_nesting);
+
+	edit = assoc_array_delete(&keyring->keys, &keyring_assoc_array_ops,
+				  &key->index_key);
+	if (IS_ERR(edit)) {
+		ret = PTR_ERR(edit);
+		goto error;
+	}
+
+	if (!edit) {
+		ret = -ENOENT;
+		goto error;
+	}
+
+	*_edit = edit;
+	return 0;
+
+error:
+	up_write(&keyring->sem);
+	return ret;
+}
+
+/*
+ * Apply an unlink change.
+ */
+static void __key_unlink(struct key *keyring, struct key *key,
+			      struct assoc_array_edit **_edit)
+{
+	assoc_array_apply_edit(*_edit);
+	*_edit = NULL;
+	key_payload_reserve(keyring, keyring->datalen - KEYQUOTA_LINK_BYTES);
+}
+
+/*
+ * Finish unlinking a key from to a keyring.
+ */
+static void __key_unlink_end(struct key *keyring,
+			     struct key *key,
+			     struct assoc_array_edit *edit)
+	__releases(&keyring->sem)
+{
+	if (edit)
+		assoc_array_cancel_edit(edit);
+	up_write(&keyring->sem);
+}
+
 /**
  * key_unlink - Unlink the first link to a key from a keyring.
  * @keyring: The keyring to remove the link from.
@@ -1407,28 +1466,13 @@ int key_unlink(struct key *keyring, struct key *key)
 	key_check(keyring);
 	key_check(key);
 
-	if (keyring->type != &key_type_keyring)
-		return -ENOTDIR;
-
-	down_write(&keyring->sem);
-
-	edit = assoc_array_delete(&keyring->keys, &keyring_assoc_array_ops,
-				  &key->index_key);
-	if (IS_ERR(edit)) {
-		ret = PTR_ERR(edit);
-		goto error;
-	}
-	ret = -ENOENT;
-	if (edit == NULL)
-		goto error;
-
-	assoc_array_apply_edit(edit);
-	key_payload_reserve(keyring, keyring->datalen - KEYQUOTA_LINK_BYTES);
-	ret = 0;
+	ret = __key_unlink_begin(keyring, 0, key, &edit);
+	if (ret < 0)
+		return ret;
 
-error:
-	up_write(&keyring->sem);
-	return ret;
+	__key_unlink(keyring, key, &edit);
+	__key_unlink_end(keyring, key, edit);
+	return 0;
 }
 EXPORT_SYMBOL(key_unlink);
 

Re: [PATCH 4/7] keys: Break bits out of key_unlink()

[James Morris]

On Wed, 22 May 2019, David Howells wrote:

> Break bits out of key_unlink() into helper functions so that they can be
> used in implementing key_move().
> 
> Signed-off-by: David Howells <dhowells@redhat.com>


Reviewed-by: James Morris <jamorris@linux.microsoft.com>


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 4/7] keys: Break bits out of key_unlink()

[James Morris]

On Wed, 22 May 2019, David Howells wrote:

> Break bits out of key_unlink() into helper functions so that they can be
> used in implementing key_move().
> 
> Signed-off-by: David Howells <dhowells@redhat.com>


Reviewed-by: James Morris <jamorris@linux.microsoft.com>


-- 
James Morris
<jmorris@namei.org>

[PATCH 5/7] keys: Make __key_link_begin() handle lockdep nesting

[David Howells]

Make __key_link_begin() handle lockdep nesting for the implementation of
key_move() where we have to lock two keyrings.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/keys/internal.h    |    2 +-
 security/keys/key.c         |    6 +++---
 security/keys/keyring.c     |    6 +++---
 security/keys/request_key.c |    2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/security/keys/internal.h b/security/keys/internal.h
index 8f533c81aa8d..93513667ff9a 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -93,7 +93,7 @@ extern wait_queue_head_t request_key_conswq;
 extern struct key_type *key_type_lookup(const char *type);
 extern void key_type_put(struct key_type *ktype);
 
-extern int __key_link_begin(struct key *keyring,
+extern int __key_link_begin(struct key *keyring, unsigned int lock_nesting,
 			    const struct keyring_index_key *index_key,
 			    struct assoc_array_edit **_edit);
 extern int __key_link_check_live_key(struct key *keyring, struct key *key);
diff --git a/security/keys/key.c b/security/keys/key.c
index 696f1c092c50..e0750bc85b68 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -515,7 +515,7 @@ int key_instantiate_and_link(struct key *key,
 	}
 
 	if (keyring) {
-		ret = __key_link_begin(keyring, &key->index_key, &edit);
+		ret = __key_link_begin(keyring, 0, &key->index_key, &edit);
 		if (ret < 0)
 			goto error;
 
@@ -583,7 +583,7 @@ int key_reject_and_link(struct key *key,
 		if (keyring->restrict_link)
 			return -EPERM;
 
-		link_ret = __key_link_begin(keyring, &key->index_key, &edit);
+		link_ret = __key_link_begin(keyring, 0, &key->index_key, &edit);
 	}
 
 	mutex_lock(&key_construction_mutex);
@@ -860,7 +860,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 	}
 	index_key.desc_len = strlen(index_key.description);
 
-	ret = __key_link_begin(keyring, &index_key, &edit);
+	ret = __key_link_begin(keyring, 0, &index_key, &edit);
 	if (ret < 0) {
 		key_ref = ERR_PTR(ret);
 		goto error_free_prep;
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 2cfeeeaa1ffa..cd669f758632 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1202,7 +1202,7 @@ static int keyring_detect_cycle(struct key *A, struct key *B)
 /*
  * Preallocate memory so that a key can be linked into to a keyring.
  */
-int __key_link_begin(struct key *keyring,
+int __key_link_begin(struct key *keyring, unsigned int lock_nesting,
 		     const struct keyring_index_key *index_key,
 		     struct assoc_array_edit **_edit)
 	__acquires(&keyring->sem)
@@ -1219,7 +1219,7 @@ int __key_link_begin(struct key *keyring,
 	if (keyring->type != &key_type_keyring)
 		return -ENOTDIR;
 
-	down_write(&keyring->sem);
+	down_write_nested(&keyring->sem, lock_nesting);
 
 	ret = -EKEYREVOKED;
 	if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))
@@ -1366,7 +1366,7 @@ int key_link(struct key *keyring, struct key *key)
 	key_check(keyring);
 	key_check(key);
 
-	ret = __key_link_begin(keyring, &key->index_key, &edit);
+	ret = __key_link_begin(keyring, 0, &key->index_key, &edit);
 	if (ret = 0) {
 		kdebug("begun {%d,%d}", keyring->serial, refcount_read(&keyring->usage));
 		ret = __key_link_check_restriction(keyring, key);
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 1f234b019437..e3653c6f85c6 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -372,7 +372,7 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
 	set_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags);
 
 	if (dest_keyring) {
-		ret = __key_link_begin(dest_keyring, &ctx->index_key, &edit);
+		ret = __key_link_begin(dest_keyring, 0, &ctx->index_key, &edit);
 		if (ret < 0)
 			goto link_prealloc_failed;
 	}

[PATCH 5/7] keys: Make __key_link_begin() handle lockdep nesting

[David Howells]

Make __key_link_begin() handle lockdep nesting for the implementation of
key_move() where we have to lock two keyrings.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/keys/internal.h    |    2 +-
 security/keys/key.c         |    6 +++---
 security/keys/keyring.c     |    6 +++---
 security/keys/request_key.c |    2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/security/keys/internal.h b/security/keys/internal.h
index 8f533c81aa8d..93513667ff9a 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -93,7 +93,7 @@ extern wait_queue_head_t request_key_conswq;
 extern struct key_type *key_type_lookup(const char *type);
 extern void key_type_put(struct key_type *ktype);
 
-extern int __key_link_begin(struct key *keyring,
+extern int __key_link_begin(struct key *keyring, unsigned int lock_nesting,
 			    const struct keyring_index_key *index_key,
 			    struct assoc_array_edit **_edit);
 extern int __key_link_check_live_key(struct key *keyring, struct key *key);
diff --git a/security/keys/key.c b/security/keys/key.c
index 696f1c092c50..e0750bc85b68 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -515,7 +515,7 @@ int key_instantiate_and_link(struct key *key,
 	}
 
 	if (keyring) {
-		ret = __key_link_begin(keyring, &key->index_key, &edit);
+		ret = __key_link_begin(keyring, 0, &key->index_key, &edit);
 		if (ret < 0)
 			goto error;
 
@@ -583,7 +583,7 @@ int key_reject_and_link(struct key *key,
 		if (keyring->restrict_link)
 			return -EPERM;
 
-		link_ret = __key_link_begin(keyring, &key->index_key, &edit);
+		link_ret = __key_link_begin(keyring, 0, &key->index_key, &edit);
 	}
 
 	mutex_lock(&key_construction_mutex);
@@ -860,7 +860,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 	}
 	index_key.desc_len = strlen(index_key.description);
 
-	ret = __key_link_begin(keyring, &index_key, &edit);
+	ret = __key_link_begin(keyring, 0, &index_key, &edit);
 	if (ret < 0) {
 		key_ref = ERR_PTR(ret);
 		goto error_free_prep;
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 2cfeeeaa1ffa..cd669f758632 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1202,7 +1202,7 @@ static int keyring_detect_cycle(struct key *A, struct key *B)
 /*
  * Preallocate memory so that a key can be linked into to a keyring.
  */
-int __key_link_begin(struct key *keyring,
+int __key_link_begin(struct key *keyring, unsigned int lock_nesting,
 		     const struct keyring_index_key *index_key,
 		     struct assoc_array_edit **_edit)
 	__acquires(&keyring->sem)
@@ -1219,7 +1219,7 @@ int __key_link_begin(struct key *keyring,
 	if (keyring->type != &key_type_keyring)
 		return -ENOTDIR;
 
-	down_write(&keyring->sem);
+	down_write_nested(&keyring->sem, lock_nesting);
 
 	ret = -EKEYREVOKED;
 	if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))
@@ -1366,7 +1366,7 @@ int key_link(struct key *keyring, struct key *key)
 	key_check(keyring);
 	key_check(key);
 
-	ret = __key_link_begin(keyring, &key->index_key, &edit);
+	ret = __key_link_begin(keyring, 0, &key->index_key, &edit);
 	if (ret == 0) {
 		kdebug("begun {%d,%d}", keyring->serial, refcount_read(&keyring->usage));
 		ret = __key_link_check_restriction(keyring, key);
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 1f234b019437..e3653c6f85c6 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -372,7 +372,7 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
 	set_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags);
 
 	if (dest_keyring) {
-		ret = __key_link_begin(dest_keyring, &ctx->index_key, &edit);
+		ret = __key_link_begin(dest_keyring, 0, &ctx->index_key, &edit);
 		if (ret < 0)
 			goto link_prealloc_failed;
 	}

Re: [PATCH 5/7] keys: Make __key_link_begin() handle lockdep nesting

[James Morris]

On Wed, 22 May 2019, David Howells wrote:

> Make __key_link_begin() handle lockdep nesting for the implementation of
> key_move() where we have to lock two keyrings.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>


Reviewed-by: James Morris <jamorris@linux.microsoft.com>

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 5/7] keys: Make __key_link_begin() handle lockdep nesting

[James Morris]

On Wed, 22 May 2019, David Howells wrote:

> Make __key_link_begin() handle lockdep nesting for the implementation of
> key_move() where we have to lock two keyrings.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>


Reviewed-by: James Morris <jamorris@linux.microsoft.com>

-- 
James Morris
<jmorris@namei.org>

[PATCH 6/7] keys: Add a keyctl to move a key between keyrings

[David Howells]

Add a keyctl to atomically move a link to a key from one keyring to
another.  The key must exist in "from" keyring and a flag can be given to
cause the operation to fail if there's a matching key already in the "to"
keyring.

This can be done with:

	keyctl(KEYCTL_MOVE,
	       key_serial_t key,
	       key_serial_t from_keyring,
	       key_serial_t to_keyring,
	       unsigned int flags);

The key being moved must grant Link permission and both keyrings must grant
Write permission.

flags should be 0 or KEYCTL_MOVE_EXCL, with the latter preventing
displacement of a matching key from the "to" keyring.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 include/linux/key.h         |    5 ++
 include/uapi/linux/keyctl.h |    3 +
 security/keys/compat.c      |    3 +
 security/keys/internal.h    |    1 
 security/keys/keyctl.c      |   55 +++++++++++++++++++++++++++
 security/keys/keyring.c     |   88 +++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 155 insertions(+)

diff --git a/include/linux/key.h b/include/linux/key.h
index 1f09aad1c98c..612e1cf84049 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -310,6 +310,11 @@ extern int key_update(key_ref_t key,
 extern int key_link(struct key *keyring,
 		    struct key *key);
 
+extern int key_move(struct key *key,
+		    struct key *from_keyring,
+		    struct key *to_keyring,
+		    unsigned int flags);
+
 extern int key_unlink(struct key *keyring,
 		      struct key *key);
 
diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
index f45ee0f69c0c..fd9fb11b312b 100644
--- a/include/uapi/linux/keyctl.h
+++ b/include/uapi/linux/keyctl.h
@@ -67,6 +67,7 @@
 #define KEYCTL_PKEY_SIGN		27	/* Create a public key signature */
 #define KEYCTL_PKEY_VERIFY		28	/* Verify a public key signature */
 #define KEYCTL_RESTRICT_KEYRING		29	/* Restrict keys allowed to link to a keyring */
+#define KEYCTL_MOVE			30	/* Move keys between keyrings */
 
 /* keyctl structures */
 struct keyctl_dh_params {
@@ -112,4 +113,6 @@ struct keyctl_pkey_params {
 	__u32		__spare[7];
 };
 
+#define KEYCTL_MOVE_EXCL	0x00000001 /* Do not displace from the to-keyring */
+
 #endif /*  _LINUX_KEYCTL_H */
diff --git a/security/keys/compat.c b/security/keys/compat.c
index 9482df601dc3..b326bc4f84d7 100644
--- a/security/keys/compat.c
+++ b/security/keys/compat.c
@@ -159,6 +159,9 @@ COMPAT_SYSCALL_DEFINE5(keyctl, u32, option,
 		return keyctl_pkey_verify(compat_ptr(arg2), compat_ptr(arg3),
 					  compat_ptr(arg4), compat_ptr(arg5));
 
+	case KEYCTL_MOVE:
+		return keyctl_keyring_move(arg2, arg3, arg4, arg5);
+
 	default:
 		return -EOPNOTSUPP;
 	}
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 93513667ff9a..821819b4ee13 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -215,6 +215,7 @@ extern long keyctl_update_key(key_serial_t, const void __user *, size_t);
 extern long keyctl_revoke_key(key_serial_t);
 extern long keyctl_keyring_clear(key_serial_t);
 extern long keyctl_keyring_link(key_serial_t, key_serial_t);
+extern long keyctl_keyring_move(key_serial_t, key_serial_t, key_serial_t, unsigned int);
 extern long keyctl_keyring_unlink(key_serial_t, key_serial_t);
 extern long keyctl_describe_key(key_serial_t, char __user *, size_t);
 extern long keyctl_keyring_search(key_serial_t, const char __user *,
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 0f947bcbad46..46188cda177e 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -572,6 +572,55 @@ long keyctl_keyring_unlink(key_serial_t id, key_serial_t ringid)
 	return ret;
 }
 
+/*
+ * Move a link to a key from one keyring to another, displacing any matching
+ * key from the destination keyring.
+ *
+ * The key must grant the caller Link permission and both keyrings must grant
+ * the caller Write permission.  There must also be a link in the from keyring
+ * to the key.  If both keyrings are the same, nothing is done.
+ *
+ * If successful, 0 will be returned.
+ */
+long keyctl_keyring_move(key_serial_t id, key_serial_t from_ringid,
+			 key_serial_t to_ringid, unsigned int flags)
+{
+	key_ref_t key_ref, from_ref, to_ref;
+	long ret;
+
+	if (flags & ~KEYCTL_MOVE_EXCL)
+		return -EINVAL;
+
+	key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE, KEY_NEED_LINK);
+	if (IS_ERR(key_ref)) {
+		ret = PTR_ERR(key_ref);
+		goto error;
+	}
+
+	from_ref = lookup_user_key(from_ringid, 0, KEY_NEED_WRITE);
+	if (IS_ERR(from_ref)) {
+		ret = PTR_ERR(from_ref);
+		goto error2;
+	}
+
+	to_ref = lookup_user_key(to_ringid, KEY_LOOKUP_CREATE, KEY_NEED_WRITE);
+	if (IS_ERR(to_ref)) {
+		ret = PTR_ERR(to_ref);
+		goto error3;
+	}
+
+	ret = key_move(key_ref_to_ptr(key_ref), key_ref_to_ptr(from_ref),
+		       key_ref_to_ptr(to_ref), flags);
+
+	key_ref_put(to_ref);
+error3:
+	key_ref_put(from_ref);
+error2:
+	key_ref_put(key_ref);
+error:
+	return ret;
+}
+
 /*
  * Return a description of a key to userspace.
  *
@@ -1772,6 +1821,12 @@ SYSCALL_DEFINE5(keyctl, int, option, unsigned long, arg2, unsigned long, arg3,
 			(const void __user *)arg4,
 			(const void __user *)arg5);
 
+	case KEYCTL_MOVE:
+		return keyctl_keyring_move((key_serial_t)arg2,
+					   (key_serial_t)arg3,
+					   (key_serial_t)arg4,
+					   (unsigned int)arg5);
+
 	default:
 		return -EOPNOTSUPP;
 	}
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index cd669f758632..df3144f9c1aa 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1476,6 +1476,94 @@ int key_unlink(struct key *keyring, struct key *key)
 }
 EXPORT_SYMBOL(key_unlink);
 
+/**
+ * key_move - Move a key from one keyring to another
+ * @key: The key to move
+ * @from_keyring: The keyring to remove the link from.
+ * @to_keyring: The keyring to make the link in.
+ * @flags: Qualifying flags, such as KEYCTL_MOVE_EXCL.
+ *
+ * Make a link in @to_keyring to a key, such that the keyring holds a reference
+ * on that key and the key can potentially be found by searching that keyring
+ * whilst simultaneously removing a link to the key from @from_keyring.
+ *
+ * This function will write-lock both keyring's semaphores and will consume
+ * some of the user's key data quota to hold the link on @to_keyring.
+ *
+ * Returns 0 if successful, -ENOTDIR if either keyring isn't a keyring,
+ * -EKEYREVOKED if either keyring has been revoked, -ENFILE if the second
+ * keyring is full, -EDQUOT if there is insufficient key data quota remaining
+ * to add another link or -ENOMEM if there's insufficient memory.  If
+ * KEYCTL_MOVE_EXCL is set, then -EEXIST will be returned if there's already a
+ * matching key in @to_keyring.
+ *
+ * It is assumed that the caller has checked that it is permitted for a link to
+ * be made (the keyring should have Write permission and the key Link
+ * permission).
+ */
+int key_move(struct key *key,
+	     struct key *from_keyring,
+	     struct key *to_keyring,
+	     unsigned int flags)
+{
+	struct assoc_array_edit *from_edit, *to_edit;
+	int ret;
+
+	kenter("%d,%d,%d", key->serial, from_keyring->serial, to_keyring->serial);
+
+	if (from_keyring = to_keyring)
+		return 0;
+
+	key_check(key);
+	key_check(from_keyring);
+	key_check(to_keyring);
+
+	/* We have to be very careful here to take the keyring locks in the
+	 * right order, lest we open ourselves to deadlocking against another
+	 * move operation.
+	 */
+	if (from_keyring < to_keyring) {
+		ret = __key_unlink_begin(from_keyring, 0, key, &from_edit);
+		if (ret < 0)
+			goto out;
+		ret = __key_link_begin(to_keyring, 1, &key->index_key, &to_edit);
+		if (ret < 0) {
+			assoc_array_cancel_edit(from_edit);
+			goto out;
+		}
+	} else {
+		ret = __key_link_begin(to_keyring, 0, &key->index_key, &to_edit);
+		if (ret < 0)
+			goto out;
+		ret = __key_unlink_begin(from_keyring, 1, key, &from_edit);
+		if (ret < 0) {
+			__key_link_end(to_keyring, &key->index_key, to_edit);
+			goto out;
+		}
+	}
+
+	ret = -EEXIST;
+	if (to_edit->dead_leaf && (flags & KEYCTL_MOVE_EXCL))
+		goto error;
+
+	ret = __key_link_check_restriction(to_keyring, key);
+	if (ret < 0)
+		goto error;
+	ret = __key_link_check_live_key(to_keyring, key);
+	if (ret < 0)
+		goto error;
+
+	__key_unlink(from_keyring, key, &from_edit);
+	__key_link(key, &to_edit);
+error:
+	__key_unlink_end(from_keyring, key, from_edit);
+	__key_link_end(to_keyring, &key->index_key, to_edit);
+out:
+	kleave(" = %d", ret);
+	return ret;
+}
+EXPORT_SYMBOL(key_move);
+
 /**
  * keyring_clear - Clear a keyring
  * @keyring: The keyring to clear.

[PATCH 6/7] keys: Add a keyctl to move a key between keyrings

[David Howells]

Add a keyctl to atomically move a link to a key from one keyring to
another.  The key must exist in "from" keyring and a flag can be given to
cause the operation to fail if there's a matching key already in the "to"
keyring.

This can be done with:

	keyctl(KEYCTL_MOVE,
	       key_serial_t key,
	       key_serial_t from_keyring,
	       key_serial_t to_keyring,
	       unsigned int flags);

The key being moved must grant Link permission and both keyrings must grant
Write permission.

flags should be 0 or KEYCTL_MOVE_EXCL, with the latter preventing
displacement of a matching key from the "to" keyring.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 include/linux/key.h         |    5 ++
 include/uapi/linux/keyctl.h |    3 +
 security/keys/compat.c      |    3 +
 security/keys/internal.h    |    1 
 security/keys/keyctl.c      |   55 +++++++++++++++++++++++++++
 security/keys/keyring.c     |   88 +++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 155 insertions(+)

diff --git a/include/linux/key.h b/include/linux/key.h
index 1f09aad1c98c..612e1cf84049 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -310,6 +310,11 @@ extern int key_update(key_ref_t key,
 extern int key_link(struct key *keyring,
 		    struct key *key);
 
+extern int key_move(struct key *key,
+		    struct key *from_keyring,
+		    struct key *to_keyring,
+		    unsigned int flags);
+
 extern int key_unlink(struct key *keyring,
 		      struct key *key);
 
diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
index f45ee0f69c0c..fd9fb11b312b 100644
--- a/include/uapi/linux/keyctl.h
+++ b/include/uapi/linux/keyctl.h
@@ -67,6 +67,7 @@
 #define KEYCTL_PKEY_SIGN		27	/* Create a public key signature */
 #define KEYCTL_PKEY_VERIFY		28	/* Verify a public key signature */
 #define KEYCTL_RESTRICT_KEYRING		29	/* Restrict keys allowed to link to a keyring */
+#define KEYCTL_MOVE			30	/* Move keys between keyrings */
 
 /* keyctl structures */
 struct keyctl_dh_params {
@@ -112,4 +113,6 @@ struct keyctl_pkey_params {
 	__u32		__spare[7];
 };
 
+#define KEYCTL_MOVE_EXCL	0x00000001 /* Do not displace from the to-keyring */
+
 #endif /*  _LINUX_KEYCTL_H */
diff --git a/security/keys/compat.c b/security/keys/compat.c
index 9482df601dc3..b326bc4f84d7 100644
--- a/security/keys/compat.c
+++ b/security/keys/compat.c
@@ -159,6 +159,9 @@ COMPAT_SYSCALL_DEFINE5(keyctl, u32, option,
 		return keyctl_pkey_verify(compat_ptr(arg2), compat_ptr(arg3),
 					  compat_ptr(arg4), compat_ptr(arg5));
 
+	case KEYCTL_MOVE:
+		return keyctl_keyring_move(arg2, arg3, arg4, arg5);
+
 	default:
 		return -EOPNOTSUPP;
 	}
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 93513667ff9a..821819b4ee13 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -215,6 +215,7 @@ extern long keyctl_update_key(key_serial_t, const void __user *, size_t);
 extern long keyctl_revoke_key(key_serial_t);
 extern long keyctl_keyring_clear(key_serial_t);
 extern long keyctl_keyring_link(key_serial_t, key_serial_t);
+extern long keyctl_keyring_move(key_serial_t, key_serial_t, key_serial_t, unsigned int);
 extern long keyctl_keyring_unlink(key_serial_t, key_serial_t);
 extern long keyctl_describe_key(key_serial_t, char __user *, size_t);
 extern long keyctl_keyring_search(key_serial_t, const char __user *,
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 0f947bcbad46..46188cda177e 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -572,6 +572,55 @@ long keyctl_keyring_unlink(key_serial_t id, key_serial_t ringid)
 	return ret;
 }
 
+/*
+ * Move a link to a key from one keyring to another, displacing any matching
+ * key from the destination keyring.
+ *
+ * The key must grant the caller Link permission and both keyrings must grant
+ * the caller Write permission.  There must also be a link in the from keyring
+ * to the key.  If both keyrings are the same, nothing is done.
+ *
+ * If successful, 0 will be returned.
+ */
+long keyctl_keyring_move(key_serial_t id, key_serial_t from_ringid,
+			 key_serial_t to_ringid, unsigned int flags)
+{
+	key_ref_t key_ref, from_ref, to_ref;
+	long ret;
+
+	if (flags & ~KEYCTL_MOVE_EXCL)
+		return -EINVAL;
+
+	key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE, KEY_NEED_LINK);
+	if (IS_ERR(key_ref)) {
+		ret = PTR_ERR(key_ref);
+		goto error;
+	}
+
+	from_ref = lookup_user_key(from_ringid, 0, KEY_NEED_WRITE);
+	if (IS_ERR(from_ref)) {
+		ret = PTR_ERR(from_ref);
+		goto error2;
+	}
+
+	to_ref = lookup_user_key(to_ringid, KEY_LOOKUP_CREATE, KEY_NEED_WRITE);
+	if (IS_ERR(to_ref)) {
+		ret = PTR_ERR(to_ref);
+		goto error3;
+	}
+
+	ret = key_move(key_ref_to_ptr(key_ref), key_ref_to_ptr(from_ref),
+		       key_ref_to_ptr(to_ref), flags);
+
+	key_ref_put(to_ref);
+error3:
+	key_ref_put(from_ref);
+error2:
+	key_ref_put(key_ref);
+error:
+	return ret;
+}
+
 /*
  * Return a description of a key to userspace.
  *
@@ -1772,6 +1821,12 @@ SYSCALL_DEFINE5(keyctl, int, option, unsigned long, arg2, unsigned long, arg3,
 			(const void __user *)arg4,
 			(const void __user *)arg5);
 
+	case KEYCTL_MOVE:
+		return keyctl_keyring_move((key_serial_t)arg2,
+					   (key_serial_t)arg3,
+					   (key_serial_t)arg4,
+					   (unsigned int)arg5);
+
 	default:
 		return -EOPNOTSUPP;
 	}
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index cd669f758632..df3144f9c1aa 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1476,6 +1476,94 @@ int key_unlink(struct key *keyring, struct key *key)
 }
 EXPORT_SYMBOL(key_unlink);
 
+/**
+ * key_move - Move a key from one keyring to another
+ * @key: The key to move
+ * @from_keyring: The keyring to remove the link from.
+ * @to_keyring: The keyring to make the link in.
+ * @flags: Qualifying flags, such as KEYCTL_MOVE_EXCL.
+ *
+ * Make a link in @to_keyring to a key, such that the keyring holds a reference
+ * on that key and the key can potentially be found by searching that keyring
+ * whilst simultaneously removing a link to the key from @from_keyring.
+ *
+ * This function will write-lock both keyring's semaphores and will consume
+ * some of the user's key data quota to hold the link on @to_keyring.
+ *
+ * Returns 0 if successful, -ENOTDIR if either keyring isn't a keyring,
+ * -EKEYREVOKED if either keyring has been revoked, -ENFILE if the second
+ * keyring is full, -EDQUOT if there is insufficient key data quota remaining
+ * to add another link or -ENOMEM if there's insufficient memory.  If
+ * KEYCTL_MOVE_EXCL is set, then -EEXIST will be returned if there's already a
+ * matching key in @to_keyring.
+ *
+ * It is assumed that the caller has checked that it is permitted for a link to
+ * be made (the keyring should have Write permission and the key Link
+ * permission).
+ */
+int key_move(struct key *key,
+	     struct key *from_keyring,
+	     struct key *to_keyring,
+	     unsigned int flags)
+{
+	struct assoc_array_edit *from_edit, *to_edit;
+	int ret;
+
+	kenter("%d,%d,%d", key->serial, from_keyring->serial, to_keyring->serial);
+
+	if (from_keyring == to_keyring)
+		return 0;
+
+	key_check(key);
+	key_check(from_keyring);
+	key_check(to_keyring);
+
+	/* We have to be very careful here to take the keyring locks in the
+	 * right order, lest we open ourselves to deadlocking against another
+	 * move operation.
+	 */
+	if (from_keyring < to_keyring) {
+		ret = __key_unlink_begin(from_keyring, 0, key, &from_edit);
+		if (ret < 0)
+			goto out;
+		ret = __key_link_begin(to_keyring, 1, &key->index_key, &to_edit);
+		if (ret < 0) {
+			assoc_array_cancel_edit(from_edit);
+			goto out;
+		}
+	} else {
+		ret = __key_link_begin(to_keyring, 0, &key->index_key, &to_edit);
+		if (ret < 0)
+			goto out;
+		ret = __key_unlink_begin(from_keyring, 1, key, &from_edit);
+		if (ret < 0) {
+			__key_link_end(to_keyring, &key->index_key, to_edit);
+			goto out;
+		}
+	}
+
+	ret = -EEXIST;
+	if (to_edit->dead_leaf && (flags & KEYCTL_MOVE_EXCL))
+		goto error;
+
+	ret = __key_link_check_restriction(to_keyring, key);
+	if (ret < 0)
+		goto error;
+	ret = __key_link_check_live_key(to_keyring, key);
+	if (ret < 0)
+		goto error;
+
+	__key_unlink(from_keyring, key, &from_edit);
+	__key_link(key, &to_edit);
+error:
+	__key_unlink_end(from_keyring, key, from_edit);
+	__key_link_end(to_keyring, &key->index_key, to_edit);
+out:
+	kleave(" = %d", ret);
+	return ret;
+}
+EXPORT_SYMBOL(key_move);
+
 /**
  * keyring_clear - Clear a keyring
  * @keyring: The keyring to clear.

Re: [PATCH 6/7] keys: Add a keyctl to move a key between keyrings

[James Morris]

On Wed, 22 May 2019, David Howells wrote:

> +
> +	if (flags & ~KEYCTL_MOVE_EXCL)
> +		return -EINVAL;
> +
> +	key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE, KEY_NEED_LINK);
> +	if (IS_ERR(key_ref)) {
> +		ret = PTR_ERR(key_ref);
> +		goto error;
> +	}

This could probably be a simple return, as there is no cleanup.

> +
> +	from_ref = lookup_user_key(from_ringid, 0, KEY_NEED_WRITE);
> +	if (IS_ERR(from_ref)) {
> +		ret = PTR_ERR(from_ref);
> +		goto error2;
> +	}
> +
> +	to_ref = lookup_user_key(to_ringid, KEY_LOOKUP_CREATE, KEY_NEED_WRITE);
> +	if (IS_ERR(to_ref)) {
> +		ret = PTR_ERR(to_ref);
> +		goto error3;
> +	}
> +
> +	ret = key_move(key_ref_to_ptr(key_ref), key_ref_to_ptr(from_ref),
> +		       key_ref_to_ptr(to_ref), flags);
> +
> +	key_ref_put(to_ref);
> +error3:
> +	key_ref_put(from_ref);
> +error2:
> +	key_ref_put(key_ref);
> +error:
> +	return ret;
> +}
> +


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 6/7] keys: Add a keyctl to move a key between keyrings

[James Morris]

On Wed, 22 May 2019, David Howells wrote:

> +
> +	if (flags & ~KEYCTL_MOVE_EXCL)
> +		return -EINVAL;
> +
> +	key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE, KEY_NEED_LINK);
> +	if (IS_ERR(key_ref)) {
> +		ret = PTR_ERR(key_ref);
> +		goto error;
> +	}

This could probably be a simple return, as there is no cleanup.

> +
> +	from_ref = lookup_user_key(from_ringid, 0, KEY_NEED_WRITE);
> +	if (IS_ERR(from_ref)) {
> +		ret = PTR_ERR(from_ref);
> +		goto error2;
> +	}
> +
> +	to_ref = lookup_user_key(to_ringid, KEY_LOOKUP_CREATE, KEY_NEED_WRITE);
> +	if (IS_ERR(to_ref)) {
> +		ret = PTR_ERR(to_ref);
> +		goto error3;
> +	}
> +
> +	ret = key_move(key_ref_to_ptr(key_ref), key_ref_to_ptr(from_ref),
> +		       key_ref_to_ptr(to_ref), flags);
> +
> +	key_ref_put(to_ref);
> +error3:
> +	key_ref_put(from_ref);
> +error2:
> +	key_ref_put(key_ref);
> +error:
> +	return ret;
> +}
> +


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 6/7] keys: Add a keyctl to move a key between keyrings

[David Howells]

James Morris <jmorris@namei.org> wrote:

> > +
> > +	if (flags & ~KEYCTL_MOVE_EXCL)
> > +		return -EINVAL;
> > +
> > +	key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE, KEY_NEED_LINK);
> > +	if (IS_ERR(key_ref)) {
> > +		ret = PTR_ERR(key_ref);
> > +		goto error;
> > +	}
> 
> This could probably be a simple return, as there is no cleanup.

Changed.

David

Re: [PATCH 6/7] keys: Add a keyctl to move a key between keyrings

[David Howells]

James Morris <jmorris@namei.org> wrote:

> > +
> > +	if (flags & ~KEYCTL_MOVE_EXCL)
> > +		return -EINVAL;
> > +
> > +	key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE, KEY_NEED_LINK);
> > +	if (IS_ERR(key_ref)) {
> > +		ret = PTR_ERR(key_ref);
> > +		goto error;
> > +	}
> 
> This could probably be a simple return, as there is no cleanup.

Changed.

David

Re: [PATCH 6/7] keys: Add a keyctl to move a key between keyrings

[Eric Biggers]

On Wed, May 22, 2019 at 11:28:45PM +0100, David Howells wrote:
> Add a keyctl to atomically move a link to a key from one keyring to
> another.  The key must exist in "from" keyring and a flag can be given to
> cause the operation to fail if there's a matching key already in the "to"
> keyring.
> 
> This can be done with:
> 
> 	keyctl(KEYCTL_MOVE,
> 	       key_serial_t key,
> 	       key_serial_t from_keyring,
> 	       key_serial_t to_keyring,
> 	       unsigned int flags);
> 
> The key being moved must grant Link permission and both keyrings must grant
> Write permission.
> 
> flags should be 0 or KEYCTL_MOVE_EXCL, with the latter preventing
> displacement of a matching key from the "to" keyring.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>

This shows up after a few seconds of syzkaller fuzzing with a description of
KEYCTL_MOVE added:

WARNING: possible circular locking dependency detected
5.2.0-rc1 #5 Not tainted
------------------------------------------------------
syz-executor.28/27700 is trying to acquire lock:
00000000049888d8 (keyring_serialise_link_sem){+.+.}, at: __key_link_begin+0x1c2/0x2d0 security/keys/keyring.c:1231

but task is already holding lock:
00000000b171310c (&type->lock_class/1){+.+.}, at: __key_link_begin+0xa4/0x2d0 security/keys/keyring.c:1222

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #1 (&type->lock_class/1){+.+.}:
       lock_acquire+0x106/0x330 kernel/locking/lockdep.c:4302
       down_write_nested+0x3c/0xa0 kernel/locking/rwsem.c:177
       __key_unlink_begin+0x6c/0x110 security/keys/keyring.c:1398
       key_move+0x3ad/0x470 security/keys/keyring.c:1538
       keyctl_keyring_move+0xb6/0x120 security/keys/keyctl.c:610
       __do_sys_keyctl security/keys/keyctl.c:1823 [inline]
       __se_sys_keyctl+0x8e/0x2c0 security/keys/keyctl.c:1685
       __x64_sys_keyctl+0xbe/0x150 security/keys/keyctl.c:1685
       do_syscall_64+0x9e/0x4b0 arch/x86/entry/common.c:301
       entry_SYSCALL_64_after_hwframe+0x49/0xbe

-> #0 (keyring_serialise_link_sem){+.+.}:
       check_prevs_add kernel/locking/lockdep.c:2417 [inline]
       validate_chain kernel/locking/lockdep.c:2799 [inline]
       __lock_acquire+0x38a4/0x3c30 kernel/locking/lockdep.c:3792
       lock_acquire+0x106/0x330 kernel/locking/lockdep.c:4302
       down_write+0x38/0xa0 kernel/locking/rwsem.c:66
       __key_link_begin+0x1c2/0x2d0 security/keys/keyring.c:1231
       key_move+0xf0/0x470 security/keys/keyring.c:1529
       keyctl_keyring_move+0xb6/0x120 security/keys/keyctl.c:610
       __do_sys_keyctl security/keys/keyctl.c:1823 [inline]
       __se_sys_keyctl+0x8e/0x2c0 security/keys/keyctl.c:1685
       __x64_sys_keyctl+0xbe/0x150 security/keys/keyctl.c:1685
       do_syscall_64+0x9e/0x4b0 arch/x86/entry/common.c:301
       entry_SYSCALL_64_after_hwframe+0x49/0xbe

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&type->lock_class/1);
                               lock(keyring_serialise_link_sem);
                               lock(&type->lock_class/1);
  lock(keyring_serialise_link_sem);

 *** DEADLOCK ***

2 locks held by syz-executor.28/27700:
 #0: 000000002a03f208 (&type->lock_class){++++}, at: __key_unlink_begin+0x6c/0x110 security/keys/keyring.c:1398
 #1: 00000000b171310c (&type->lock_class/1){+.+.}, at: __key_link_begin+0xa4/0x2d0 security/keys/keyring.c:1222

stack backtrace:
CPU: 8 PID: 27700 Comm: syz-executor.28 Not tainted 5.2.0-rc1 #5
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0xb1/0x118 lib/dump_stack.c:113
 print_circular_bug+0x4a4/0x4b5 kernel/locking/lockdep.c:1564
 check_prev_add+0xd1f/0x1af7 kernel/locking/lockdep.c:2309
 check_prevs_add kernel/locking/lockdep.c:2417 [inline]
 validate_chain kernel/locking/lockdep.c:2799 [inline]
 __lock_acquire+0x38a4/0x3c30 kernel/locking/lockdep.c:3792
 lock_acquire+0x106/0x330 kernel/locking/lockdep.c:4302
 down_write+0x38/0xa0 kernel/locking/rwsem.c:66
 __key_link_begin+0x1c2/0x2d0 security/keys/keyring.c:1231
 key_move+0xf0/0x470 security/keys/keyring.c:1529
 keyctl_keyring_move+0xb6/0x120 security/keys/keyctl.c:610
 __do_sys_keyctl security/keys/keyctl.c:1823 [inline]
 __se_sys_keyctl+0x8e/0x2c0 security/keys/keyctl.c:1685
 __x64_sys_keyctl+0xbe/0x150 security/keys/keyctl.c:1685
 do_syscall_64+0x9e/0x4b0 arch/x86/entry/common.c:301
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x458a09
Code: dd b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 ab b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f9d53755c88 EFLAGS: 00000246 ORIG_RAX: 00000000000000fa
RAX: ffffffffffffffda RBX: 000000000071bf00 RCX: 0000000000458a09
RDX: 000000001f62d16e RSI: 000000002490e642 RDI: 000000000000001e
RBP: 00007f9d53755ca0 R08: 0000000000000000 R09: 0000000000000000
R10: 000000001afbc80a R11: 0000000000000246 R12: 00007f9d537566d4
R13: 00000000004ac12c R14: 00000000006ebd68 R15: 0000000000000003
FAULT_INJECTION: forcing a failure.
name failslab, interval 1, probability 0, space 0, times 0
CPU: 8 PID: 27700 Comm: syz-executor.28 Not tainted 5.2.0-rc1 #5
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0xb1/0x118 lib/dump_stack.c:113
 fail_dump lib/fault-inject.c:51 [inline]
 should_fail+0x61e/0x720 lib/fault-inject.c:143
 __should_failslab+0xec/0x120 mm/failslab.c:32
 should_failslab+0x9/0x14 mm/slab_common.c:1610
 slab_pre_alloc_hook mm/slab.h:420 [inline]
 slab_alloc mm/slab.c:3312 [inline]
 kmem_cache_alloc_trace+0x146/0x2a0 mm/slab.c:3553
 kmalloc include/linux/slab.h:547 [inline]
 kzalloc include/linux/slab.h:742 [inline]
 assoc_array_insert+0xcc/0x440 lib/assoc_array.c:985
 __key_link_begin+0x120/0x2d0 security/keys/keyring.c:1236
 key_move+0xf0/0x470 security/keys/keyring.c:1529
 keyctl_keyring_move+0xb6/0x120 security/keys/keyctl.c:610
 __do_sys_keyctl security/keys/keyctl.c:1823 [inline]
 __se_sys_keyctl+0x8e/0x2c0 security/keys/keyctl.c:1685
 __x64_sys_keyctl+0xbe/0x150 security/keys/keyctl.c:1685
 do_syscall_64+0x9e/0x4b0 arch/x86/entry/common.c:301
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x458a09
Code: dd b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 ab b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f9d53755c88 EFLAGS: 00000246 ORIG_RAX: 00000000000000fa
RAX: ffffffffffffffda RBX: 000000000071bf00 RCX: 0000000000458a09
RDX: 000000001f62d16e RSI: 000000002490e642 RDI: 000000000000001e
RBP: 00007f9d53755ca0 R08: 0000000000000000 R09: 0000000000000000
R10: 000000001afbc80a R11: 0000000000000246 R12: 00007f9d537566d4
R13: 00000000004ac12c R14: 00000000006ebd68 R15: 0000000000000003

Re: [PATCH 6/7] keys: Add a keyctl to move a key between keyrings

[Eric Biggers]

On Wed, May 22, 2019 at 11:28:45PM +0100, David Howells wrote:
> Add a keyctl to atomically move a link to a key from one keyring to
> another.  The key must exist in "from" keyring and a flag can be given to
> cause the operation to fail if there's a matching key already in the "to"
> keyring.
> 
> This can be done with:
> 
> 	keyctl(KEYCTL_MOVE,
> 	       key_serial_t key,
> 	       key_serial_t from_keyring,
> 	       key_serial_t to_keyring,
> 	       unsigned int flags);
> 
> The key being moved must grant Link permission and both keyrings must grant
> Write permission.
> 
> flags should be 0 or KEYCTL_MOVE_EXCL, with the latter preventing
> displacement of a matching key from the "to" keyring.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>

This shows up after a few seconds of syzkaller fuzzing with a description of
KEYCTL_MOVE added:

WARNING: possible circular locking dependency detected
5.2.0-rc1 #5 Not tainted
------------------------------------------------------
syz-executor.28/27700 is trying to acquire lock:
00000000049888d8 (keyring_serialise_link_sem){+.+.}, at: __key_link_begin+0x1c2/0x2d0 security/keys/keyring.c:1231

but task is already holding lock:
00000000b171310c (&type->lock_class/1){+.+.}, at: __key_link_begin+0xa4/0x2d0 security/keys/keyring.c:1222

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #1 (&type->lock_class/1){+.+.}:
       lock_acquire+0x106/0x330 kernel/locking/lockdep.c:4302
       down_write_nested+0x3c/0xa0 kernel/locking/rwsem.c:177
       __key_unlink_begin+0x6c/0x110 security/keys/keyring.c:1398
       key_move+0x3ad/0x470 security/keys/keyring.c:1538
       keyctl_keyring_move+0xb6/0x120 security/keys/keyctl.c:610
       __do_sys_keyctl security/keys/keyctl.c:1823 [inline]
       __se_sys_keyctl+0x8e/0x2c0 security/keys/keyctl.c:1685
       __x64_sys_keyctl+0xbe/0x150 security/keys/keyctl.c:1685
       do_syscall_64+0x9e/0x4b0 arch/x86/entry/common.c:301
       entry_SYSCALL_64_after_hwframe+0x49/0xbe

-> #0 (keyring_serialise_link_sem){+.+.}:
       check_prevs_add kernel/locking/lockdep.c:2417 [inline]
       validate_chain kernel/locking/lockdep.c:2799 [inline]
       __lock_acquire+0x38a4/0x3c30 kernel/locking/lockdep.c:3792
       lock_acquire+0x106/0x330 kernel/locking/lockdep.c:4302
       down_write+0x38/0xa0 kernel/locking/rwsem.c:66
       __key_link_begin+0x1c2/0x2d0 security/keys/keyring.c:1231
       key_move+0xf0/0x470 security/keys/keyring.c:1529
       keyctl_keyring_move+0xb6/0x120 security/keys/keyctl.c:610
       __do_sys_keyctl security/keys/keyctl.c:1823 [inline]
       __se_sys_keyctl+0x8e/0x2c0 security/keys/keyctl.c:1685
       __x64_sys_keyctl+0xbe/0x150 security/keys/keyctl.c:1685
       do_syscall_64+0x9e/0x4b0 arch/x86/entry/common.c:301
       entry_SYSCALL_64_after_hwframe+0x49/0xbe

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&type->lock_class/1);
                               lock(keyring_serialise_link_sem);
                               lock(&type->lock_class/1);
  lock(keyring_serialise_link_sem);

 *** DEADLOCK ***

2 locks held by syz-executor.28/27700:
 #0: 000000002a03f208 (&type->lock_class){++++}, at: __key_unlink_begin+0x6c/0x110 security/keys/keyring.c:1398
 #1: 00000000b171310c (&type->lock_class/1){+.+.}, at: __key_link_begin+0xa4/0x2d0 security/keys/keyring.c:1222

stack backtrace:
CPU: 8 PID: 27700 Comm: syz-executor.28 Not tainted 5.2.0-rc1 #5
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0xb1/0x118 lib/dump_stack.c:113
 print_circular_bug+0x4a4/0x4b5 kernel/locking/lockdep.c:1564
 check_prev_add+0xd1f/0x1af7 kernel/locking/lockdep.c:2309
 check_prevs_add kernel/locking/lockdep.c:2417 [inline]
 validate_chain kernel/locking/lockdep.c:2799 [inline]
 __lock_acquire+0x38a4/0x3c30 kernel/locking/lockdep.c:3792
 lock_acquire+0x106/0x330 kernel/locking/lockdep.c:4302
 down_write+0x38/0xa0 kernel/locking/rwsem.c:66
 __key_link_begin+0x1c2/0x2d0 security/keys/keyring.c:1231
 key_move+0xf0/0x470 security/keys/keyring.c:1529
 keyctl_keyring_move+0xb6/0x120 security/keys/keyctl.c:610
 __do_sys_keyctl security/keys/keyctl.c:1823 [inline]
 __se_sys_keyctl+0x8e/0x2c0 security/keys/keyctl.c:1685
 __x64_sys_keyctl+0xbe/0x150 security/keys/keyctl.c:1685
 do_syscall_64+0x9e/0x4b0 arch/x86/entry/common.c:301
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x458a09
Code: dd b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 ab b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f9d53755c88 EFLAGS: 00000246 ORIG_RAX: 00000000000000fa
RAX: ffffffffffffffda RBX: 000000000071bf00 RCX: 0000000000458a09
RDX: 000000001f62d16e RSI: 000000002490e642 RDI: 000000000000001e
RBP: 00007f9d53755ca0 R08: 0000000000000000 R09: 0000000000000000
R10: 000000001afbc80a R11: 0000000000000246 R12: 00007f9d537566d4
R13: 00000000004ac12c R14: 00000000006ebd68 R15: 0000000000000003
FAULT_INJECTION: forcing a failure.
name failslab, interval 1, probability 0, space 0, times 0
CPU: 8 PID: 27700 Comm: syz-executor.28 Not tainted 5.2.0-rc1 #5
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0xb1/0x118 lib/dump_stack.c:113
 fail_dump lib/fault-inject.c:51 [inline]
 should_fail+0x61e/0x720 lib/fault-inject.c:143
 __should_failslab+0xec/0x120 mm/failslab.c:32
 should_failslab+0x9/0x14 mm/slab_common.c:1610
 slab_pre_alloc_hook mm/slab.h:420 [inline]
 slab_alloc mm/slab.c:3312 [inline]
 kmem_cache_alloc_trace+0x146/0x2a0 mm/slab.c:3553
 kmalloc include/linux/slab.h:547 [inline]
 kzalloc include/linux/slab.h:742 [inline]
 assoc_array_insert+0xcc/0x440 lib/assoc_array.c:985
 __key_link_begin+0x120/0x2d0 security/keys/keyring.c:1236
 key_move+0xf0/0x470 security/keys/keyring.c:1529
 keyctl_keyring_move+0xb6/0x120 security/keys/keyctl.c:610
 __do_sys_keyctl security/keys/keyctl.c:1823 [inline]
 __se_sys_keyctl+0x8e/0x2c0 security/keys/keyctl.c:1685
 __x64_sys_keyctl+0xbe/0x150 security/keys/keyctl.c:1685
 do_syscall_64+0x9e/0x4b0 arch/x86/entry/common.c:301
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x458a09
Code: dd b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 ab b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f9d53755c88 EFLAGS: 00000246 ORIG_RAX: 00000000000000fa
RAX: ffffffffffffffda RBX: 000000000071bf00 RCX: 0000000000458a09
RDX: 000000001f62d16e RSI: 000000002490e642 RDI: 000000000000001e
RBP: 00007f9d53755ca0 R08: 0000000000000000 R09: 0000000000000000
R10: 000000001afbc80a R11: 0000000000000246 R12: 00007f9d537566d4
R13: 00000000004ac12c R14: 00000000006ebd68 R15: 0000000000000003

Re: [PATCH 6/7] keys: Add a keyctl to move a key between keyrings

[David Howells]

Eric Biggers <ebiggers@kernel.org> wrote:

> This shows up after a few seconds of syzkaller fuzzing with a description of
> KEYCTL_MOVE added:

Yeah...  I'm fixing that now.  I've also created a bunch of tests, manpages,
etc. for keyutils which I'll push when I've fixed my patches.

David

Re: [PATCH 6/7] keys: Add a keyctl to move a key between keyrings

[David Howells]

Eric Biggers <ebiggers@kernel.org> wrote:

> This shows up after a few seconds of syzkaller fuzzing with a description of
> KEYCTL_MOVE added:

Yeah...  I'm fixing that now.  I've also created a bunch of tests, manpages,
etc. for keyutils which I'll push when I've fixed my patches.

David

[PATCH 7/7] keys: Grant Link permission to possessers of request_key auth keys

[David Howells]

Grant Link permission to the possessers of request_key authentication keys,
thereby allowing a daemon that is servicing upcalls to arrange things such
that only the necessary auth key is passed to the actual service program
and not all the daemon's pending auth keys.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/keys/request_key_auth.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index 572c7a60473a..ec5226557023 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -204,7 +204,7 @@ struct key *request_key_auth_new(struct key *target, const char *op,
 
 	authkey = key_alloc(&key_type_request_key_auth, desc,
 			    cred->fsuid, cred->fsgid, cred,
-			    KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH |
+			    KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH | KEY_POS_LINK |
 			    KEY_USR_VIEW, KEY_ALLOC_NOT_IN_QUOTA, NULL);
 	if (IS_ERR(authkey)) {
 		ret = PTR_ERR(authkey);

[PATCH 7/7] keys: Grant Link permission to possessers of request_key auth keys

[David Howells]

Grant Link permission to the possessers of request_key authentication keys,
thereby allowing a daemon that is servicing upcalls to arrange things such
that only the necessary auth key is passed to the actual service program
and not all the daemon's pending auth keys.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/keys/request_key_auth.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index 572c7a60473a..ec5226557023 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -204,7 +204,7 @@ struct key *request_key_auth_new(struct key *target, const char *op,
 
 	authkey = key_alloc(&key_type_request_key_auth, desc,
 			    cred->fsuid, cred->fsgid, cred,
-			    KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH |
+			    KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH | KEY_POS_LINK |
 			    KEY_USR_VIEW, KEY_ALLOC_NOT_IN_QUOTA, NULL);
 	if (IS_ERR(authkey)) {
 		ret = PTR_ERR(authkey);

Re: [PATCH 7/7] keys: Grant Link permission to possessers of request_key auth keys

[James Morris]

On Wed, 22 May 2019, David Howells wrote:

> Grant Link permission to the possessers of request_key authentication keys,
> thereby allowing a daemon that is servicing upcalls to arrange things such
> that only the necessary auth key is passed to the actual service program
> and not all the daemon's pending auth keys.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>


Reviewed-by: James Morris <jamorris@linux.microsoft.com>


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 7/7] keys: Grant Link permission to possessers of request_key auth keys

[James Morris]

On Wed, 22 May 2019, David Howells wrote:

> Grant Link permission to the possessers of request_key authentication keys,
> thereby allowing a daemon that is servicing upcalls to arrange things such
> that only the necessary auth key is passed to the actual service program
> and not all the daemon's pending auth keys.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>


Reviewed-by: James Morris <jamorris@linux.microsoft.com>


-- 
James Morris
<jmorris@namei.org>