[PATCH v2 0/2] get rid of cred_transfer

[PATCH v2 0/2] get rid of cred_transfer

[Jann Horn]

This is the approach I proposed at
<https://lore.kernel.org/all/CAG48ez2bnvuX8i-D=5DxmfzEOKTWAf-DkgQq6aNC4WzSGoEGHg@mail.gmail.com/>
to get rid of the cred_transfer stuff.

What do you think? Synchronously waiting for task work is a bit ugly,
but at least this condenses the uglyness in the keys subsystem instead
of making the rest of the security subsystem deal with this stuff.

Another approach to simplify things further would be to try to move
the session keyring out of the creds entirely and just let the child
update it directly with appropriate locking, but I don't know enough
about the keys subsystem to know if that would maybe break stuff
that relies on override_creds() also overriding the keyrings, or
something like that.

Signed-off-by: Jann Horn <jannh@google.com>
---
Changes in v2:
- use interruptible wait instead of killable
- split into two patches (Jarkko)
- Link to v1: https://lore.kernel.org/r/20240802-remove-cred-transfer-v1-1-b3fef1ef2ade@google.com

---
Jann Horn (2):
      KEYS: use synchronous task work for changing parent credentials
      security: remove unused cred_alloc_blank/cred_transfer helpers

 include/linux/cred.h          |   1 -
 include/linux/lsm_hook_defs.h |   3 --
 include/linux/security.h      |  12 -----
 kernel/cred.c                 |  23 ---------
 security/apparmor/lsm.c       |  19 --------
 security/keys/internal.h      |   8 ++++
 security/keys/keyctl.c        | 107 +++++++++++++-----------------------------
 security/keys/process_keys.c  |  86 +++++++++++++++++----------------
 security/landlock/cred.c      |  11 +----
 security/security.c           |  35 --------------
 security/selinux/hooks.c      |  12 -----
 security/smack/smack_lsm.c    |  32 -------------
 12 files changed, 89 insertions(+), 260 deletions(-)
---
base-commit: c0ecd6388360d930440cc5554026818895199923
change-id: 20240802-remove-cred-transfer-493a3b696da2
-- 
Jann Horn <jannh@google.com>

[PATCH v2 1/2] KEYS: use synchronous task work for changing parent credentials

[Jann Horn]

keyctl_session_to_parent() involves posting task work to the parent task,
with work function key_change_session_keyring.
Because the task work in the parent runs asynchronously, no errors can be
returned back to the caller of keyctl_session_to_parent(), and therefore
the work function key_change_session_keyring() can't be allowed to fail due
to things like memory allocation failure or permission checks - all
allocations and checks have to happen in the child.

This is annoying for two reasons:

 - It is the only reason why cred_alloc_blank() and
   security_transfer_creds() are necessary.
 - It means we can't do synchronous permission checks.

Rewrite keyctl_session_to_parent() to run task work on the parent
synchronously, so that any errors that happen in the task work can be
plumbed back into the syscall return value in the child.
This allows us to get rid of cred_alloc_blank() and
security_transfer_creds() in a later commit, and it will make it possible
to write more reliable security checks for this operation.

Note that this requires using TWA_SIGNAL instead of TWA_RESUME, so the
parent might observe some spurious -EAGAIN syscall returns or such; but the
parent likely anyway has to be ready to deal with the side effects of
receiving signals (since it'll probably get SIGCHLD when the child dies),
so that probably isn't an issue.

Signed-off-by: Jann Horn <jannh@google.com>
---
 security/keys/internal.h     |   8 ++++
 security/keys/keyctl.c       | 107 +++++++++++++------------------------------
 security/keys/process_keys.c |  86 ++++++++++++++++++----------------
 3 files changed, 87 insertions(+), 114 deletions(-)

diff --git a/security/keys/internal.h b/security/keys/internal.h
index 2cffa6dc8255..2c5eadc04cf2 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -157,12 +157,20 @@ extern struct key *request_key_and_link(struct key_type *type,
 					unsigned long flags);
 
 extern bool lookup_user_key_possessed(const struct key *key,
 				      const struct key_match_data *match_data);
 
 extern long join_session_keyring(const char *name);
+
+struct keyctl_session_to_parent_context {
+	struct callback_head work;
+	struct completion done;
+	struct key *new_session_keyring;
+	const struct cred *child_cred;
+	int result;
+};
 extern void key_change_session_keyring(struct callback_head *twork);
 
 extern struct work_struct key_gc_work;
 extern unsigned key_gc_delay;
 extern void keyring_gc(struct key *keyring, time64_t limit);
 extern void keyring_restriction_gc(struct key *keyring,
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index ab927a142f51..e4cfe5c4594a 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1616,104 +1616,63 @@ long keyctl_get_security(key_serial_t keyid,
  * parent process.
  *
  * The keyring must exist and must grant the caller LINK permission, and the
  * parent process must be single-threaded and must have the same effective
  * ownership as this process and mustn't be SUID/SGID.
  *
- * The keyring will be emplaced on the parent when it next resumes userspace.
+ * The keyring will be emplaced on the parent via a pseudo-signal.
  *
  * If successful, 0 will be returned.
  */
 long keyctl_session_to_parent(void)
 {
-	struct task_struct *me, *parent;
-	const struct cred *mycred, *pcred;
-	struct callback_head *newwork, *oldwork;
+	struct keyctl_session_to_parent_context ctx;
+	struct task_struct *parent;
 	key_ref_t keyring_r;
-	struct cred *cred;
 	int ret;
 
 	keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING, 0, KEY_NEED_LINK);
 	if (IS_ERR(keyring_r))
 		return PTR_ERR(keyring_r);
 
-	ret = -ENOMEM;
-
-	/* our parent is going to need a new cred struct, a new tgcred struct
-	 * and new security data, so we allocate them here to prevent ENOMEM in
-	 * our parent */
-	cred = cred_alloc_blank();
-	if (!cred)
-		goto error_keyring;
-	newwork = &cred->rcu;
+	write_lock_irq(&tasklist_lock);
+	parent = get_task_struct(rcu_dereference_protected(current->real_parent,
+					lockdep_is_held(&tasklist_lock)));
+	write_unlock_irq(&tasklist_lock);
 
-	cred->session_keyring = key_ref_to_ptr(keyring_r);
-	keyring_r = NULL;
-	init_task_work(newwork, key_change_session_keyring);
+	/* the parent mustn't be init and mustn't be a kernel thread */
+	if (is_global_init(parent) || (READ_ONCE(parent->flags) & PF_KTHREAD) != 0)
+		goto put_task;
 
-	me = current;
-	rcu_read_lock();
-	write_lock_irq(&tasklist_lock);
+	ctx.new_session_keyring = key_ref_to_ptr(keyring_r);
+	ctx.child_cred = current_cred();
+	init_completion(&ctx.done);
+	init_task_work(&ctx.work, key_change_session_keyring);
+	ret = task_work_add(parent, &ctx.work, TWA_SIGNAL);
+	if (ret)
+		goto put_task;
 
-	ret = -EPERM;
-	oldwork = NULL;
-	parent = rcu_dereference_protected(me->real_parent,
-					   lockdep_is_held(&tasklist_lock));
+	ret = wait_for_completion_interruptible(&ctx.done);
 
-	/* the parent mustn't be init and mustn't be a kernel thread */
-	if (parent->pid <= 1 || !parent->mm)
-		goto unlock;
-
-	/* the parent must be single threaded */
-	if (!thread_group_empty(parent))
-		goto unlock;
-
-	/* the parent and the child must have different session keyrings or
-	 * there's no point */
-	mycred = current_cred();
-	pcred = __task_cred(parent);
-	if (mycred == pcred ||
-	    mycred->session_keyring == pcred->session_keyring) {
-		ret = 0;
-		goto unlock;
+	if (task_work_cancel(parent, &ctx.work)) {
+		/*
+		 * We got interrupted and the task work was canceled before it
+		 * could execute.
+		 * Use -ERESTARTNOINTR instead of -ERESTARTSYS for
+		 * compatibility - the manpage does not list -EINTR as a
+		 * possible error for keyctl().
+		 */
+		ret = -ERESTARTNOINTR;
+	} else {
+		/* task work is running or has been executed */
+		wait_for_completion(&ctx.done);
+		ret = ctx.result;
 	}
 
-	/* the parent must have the same effective ownership and mustn't be
-	 * SUID/SGID */
-	if (!uid_eq(pcred->uid,	 mycred->euid) ||
-	    !uid_eq(pcred->euid, mycred->euid) ||
-	    !uid_eq(pcred->suid, mycred->euid) ||
-	    !gid_eq(pcred->gid,	 mycred->egid) ||
-	    !gid_eq(pcred->egid, mycred->egid) ||
-	    !gid_eq(pcred->sgid, mycred->egid))
-		goto unlock;
-
-	/* the keyrings must have the same UID */
-	if ((pcred->session_keyring &&
-	     !uid_eq(pcred->session_keyring->uid, mycred->euid)) ||
-	    !uid_eq(mycred->session_keyring->uid, mycred->euid))
-		goto unlock;
-
-	/* cancel an already pending keyring replacement */
-	oldwork = task_work_cancel_func(parent, key_change_session_keyring);
-
-	/* the replacement session keyring is applied just prior to userspace
-	 * restarting */
-	ret = task_work_add(parent, newwork, TWA_RESUME);
-	if (!ret)
-		newwork = NULL;
-unlock:
-	write_unlock_irq(&tasklist_lock);
-	rcu_read_unlock();
-	if (oldwork)
-		put_cred(container_of(oldwork, struct cred, rcu));
-	if (newwork)
-		put_cred(cred);
-	return ret;
-
-error_keyring:
+put_task:
+	put_task_struct(parent);
 	key_ref_put(keyring_r);
 	return ret;
 }
 
 /*
  * Apply a restriction to a given keyring.
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index b5d5333ab330..199c5dd34792 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -902,59 +902,65 @@ long join_session_keyring(const char *name)
 error:
 	abort_creds(new);
 	return ret;
 }
 
 /*
- * Replace a process's session keyring on behalf of one of its children when
- * the target  process is about to resume userspace execution.
+ * Replace a process's session keyring on behalf of one of its children.
+ * This function runs in task context, while the child is blocked in
+ * keyctl_session_to_parent().
  */
-void key_change_session_keyring(struct callback_head *twork)
+void key_change_session_keyring(struct callback_head *work)
 {
-	const struct cred *old = current_cred();
-	struct cred *new = container_of(twork, struct cred, rcu);
+	struct keyctl_session_to_parent_context *ctx =
+		container_of(work, struct keyctl_session_to_parent_context, work);
+	const struct cred *pcred = current_cred();
+	const struct cred *ccred = ctx->child_cred;
+	struct cred *new;
 
-	if (unlikely(current->flags & PF_EXITING)) {
-		put_cred(new);
-		return;
-	}
+	/* do checks */
+	ctx->result = -EPERM;
+	if (unlikely(current->flags & PF_EXITING))
+		goto out;
 
-	/* If get_ucounts fails more bits are needed in the refcount */
-	if (unlikely(!get_ucounts(old->ucounts))) {
-		WARN_ONCE(1, "In %s get_ucounts failed\n", __func__);
-		put_cred(new);
-		return;
-	}
+	/* we must be single threaded */
+	if (!thread_group_empty(current))
+		goto out;
+
+	/*
+	 * the parent must have the same effective ownership and mustn't be
+	 * SUID/SGID
+	 */
+	if (!uid_eq(pcred->uid,	 ccred->euid) ||
+	    !uid_eq(pcred->euid, ccred->euid) ||
+	    !uid_eq(pcred->suid, ccred->euid) ||
+	    !gid_eq(pcred->gid,	 ccred->egid) ||
+	    !gid_eq(pcred->egid, ccred->egid) ||
+	    !gid_eq(pcred->sgid, ccred->egid))
+		goto out;
+
+	/* the keyrings must have the same UID */
+	if ((pcred->session_keyring &&
+	     !uid_eq(pcred->session_keyring->uid, ccred->euid)) ||
+	    !uid_eq(ctx->new_session_keyring->uid, ccred->euid))
+		goto out;
+
+
+	/* okay, try to update creds */
+	ctx->result = -ENOMEM;
+	new = prepare_creds();
+	if (!new)
+		goto out;
 
-	new->  uid	= old->  uid;
-	new-> euid	= old-> euid;
-	new-> suid	= old-> suid;
-	new->fsuid	= old->fsuid;
-	new->  gid	= old->  gid;
-	new-> egid	= old-> egid;
-	new-> sgid	= old-> sgid;
-	new->fsgid	= old->fsgid;
-	new->user	= get_uid(old->user);
-	new->ucounts	= old->ucounts;
-	new->user_ns	= get_user_ns(old->user_ns);
-	new->group_info	= get_group_info(old->group_info);
-
-	new->securebits	= old->securebits;
-	new->cap_inheritable	= old->cap_inheritable;
-	new->cap_permitted	= old->cap_permitted;
-	new->cap_effective	= old->cap_effective;
-	new->cap_ambient	= old->cap_ambient;
-	new->cap_bset		= old->cap_bset;
-
-	new->jit_keyring	= old->jit_keyring;
-	new->thread_keyring	= key_get(old->thread_keyring);
-	new->process_keyring	= key_get(old->process_keyring);
-
-	security_transfer_creds(new, old);
+	key_put(new->session_keyring);
+	new->session_keyring = key_get(ctx->new_session_keyring);
 
 	commit_creds(new);
+	ctx->result = 0;
+out:
+	complete_all(&ctx->done);
 }
 
 /*
  * Make sure that root's user and user-session keyrings exist.
  */
 static int __init init_root_keyring(void)

-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH v2 2/2] security: remove unused cred_alloc_blank/cred_transfer helpers

[Jann Horn]

Now that they're unused thanks to the preceding commit, remove
cred_alloc_blank(), security_transfer_creds(), and the corresponding LSM
hook implementations.

Signed-off-by: Jann Horn <jannh@google.com>
---
 include/linux/cred.h          |  1 -
 include/linux/lsm_hook_defs.h |  3 ---
 include/linux/security.h      | 12 ------------
 kernel/cred.c                 | 23 -----------------------
 security/apparmor/lsm.c       | 19 -------------------
 security/landlock/cred.c      | 11 ++---------
 security/security.c           | 35 -----------------------------------
 security/selinux/hooks.c      | 12 ------------
 security/smack/smack_lsm.c    | 32 --------------------------------
 9 files changed, 2 insertions(+), 146 deletions(-)

diff --git a/include/linux/cred.h b/include/linux/cred.h
index 2976f534a7a3..54b5105d4cd5 100644
--- a/include/linux/cred.h
+++ b/include/linux/cred.h
@@ -147,13 +147,12 @@ struct cred {
 } __randomize_layout;
 
 extern void __put_cred(struct cred *);
 extern void exit_creds(struct task_struct *);
 extern int copy_creds(struct task_struct *, unsigned long);
 extern const struct cred *get_task_cred(struct task_struct *);
-extern struct cred *cred_alloc_blank(void);
 extern struct cred *prepare_creds(void);
 extern struct cred *prepare_exec_creds(void);
 extern int commit_creds(struct cred *);
 extern void abort_creds(struct cred *);
 extern const struct cred *override_creds(const struct cred *);
 extern void revert_creds(const struct cred *);
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 855db460e08b..1d75075cb607 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -204,18 +204,15 @@ LSM_HOOK(int, 0, file_receive, struct file *file)
 LSM_HOOK(int, 0, file_open, struct file *file)
 LSM_HOOK(int, 0, file_post_open, struct file *file, int mask)
 LSM_HOOK(int, 0, file_truncate, struct file *file)
 LSM_HOOK(int, 0, task_alloc, struct task_struct *task,
 	 unsigned long clone_flags)
 LSM_HOOK(void, LSM_RET_VOID, task_free, struct task_struct *task)
-LSM_HOOK(int, 0, cred_alloc_blank, struct cred *cred, gfp_t gfp)
 LSM_HOOK(void, LSM_RET_VOID, cred_free, struct cred *cred)
 LSM_HOOK(int, 0, cred_prepare, struct cred *new, const struct cred *old,
 	 gfp_t gfp)
-LSM_HOOK(void, LSM_RET_VOID, cred_transfer, struct cred *new,
-	 const struct cred *old)
 LSM_HOOK(void, LSM_RET_VOID, cred_getsecid, const struct cred *c, u32 *secid)
 LSM_HOOK(int, 0, kernel_act_as, struct cred *new, u32 secid)
 LSM_HOOK(int, 0, kernel_create_files_as, struct cred *new, struct inode *inode)
 LSM_HOOK(int, 0, kernel_module_request, char *kmod_name)
 LSM_HOOK(int, 0, kernel_load_data, enum kernel_load_data_id id, bool contents)
 LSM_HOOK(int, 0, kernel_post_load_data, char *buf, loff_t size,
diff --git a/include/linux/security.h b/include/linux/security.h
index 1390f1efb4f0..a366c2a03f55 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -421,16 +421,14 @@ int security_file_send_sigiotask(struct task_struct *tsk,
 int security_file_receive(struct file *file);
 int security_file_open(struct file *file);
 int security_file_post_open(struct file *file, int mask);
 int security_file_truncate(struct file *file);
 int security_task_alloc(struct task_struct *task, unsigned long clone_flags);
 void security_task_free(struct task_struct *task);
-int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
 void security_cred_free(struct cred *cred);
 int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp);
-void security_transfer_creds(struct cred *new, const struct cred *old);
 void security_cred_getsecid(const struct cred *c, u32 *secid);
 int security_kernel_act_as(struct cred *new, u32 secid);
 int security_kernel_create_files_as(struct cred *new, struct inode *inode);
 int security_kernel_module_request(char *kmod_name);
 int security_kernel_load_data(enum kernel_load_data_id id, bool contents);
 int security_kernel_post_load_data(char *buf, loff_t size,
@@ -1117,32 +1115,22 @@ static inline int security_task_alloc(struct task_struct *task,
 	return 0;
 }
 
 static inline void security_task_free(struct task_struct *task)
 { }
 
-static inline int security_cred_alloc_blank(struct cred *cred, gfp_t gfp)
-{
-	return 0;
-}
-
 static inline void security_cred_free(struct cred *cred)
 { }
 
 static inline int security_prepare_creds(struct cred *new,
 					 const struct cred *old,
 					 gfp_t gfp)
 {
 	return 0;
 }
 
-static inline void security_transfer_creds(struct cred *new,
-					   const struct cred *old)
-{
-}
-
 static inline void security_cred_getsecid(const struct cred *c, u32 *secid)
 {
 	*secid = 0;
 }
 
 static inline int security_kernel_act_as(struct cred *cred, u32 secid)
diff --git a/kernel/cred.c b/kernel/cred.c
index 075cfa7c896f..b2f6130cd6b7 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -163,35 +163,12 @@ const struct cred *get_task_cred(struct task_struct *task)
 
 	rcu_read_unlock();
 	return cred;
 }
 EXPORT_SYMBOL(get_task_cred);
 
-/*
- * Allocate blank credentials, such that the credentials can be filled in at a
- * later date without risk of ENOMEM.
- */
-struct cred *cred_alloc_blank(void)
-{
-	struct cred *new;
-
-	new = kmem_cache_zalloc(cred_jar, GFP_KERNEL);
-	if (!new)
-		return NULL;
-
-	atomic_long_set(&new->usage, 1);
-	if (security_cred_alloc_blank(new, GFP_KERNEL_ACCOUNT) < 0)
-		goto error;
-
-	return new;
-
-error:
-	abort_creds(new);
-	return NULL;
-}
-
 /**
  * prepare_creds - Prepare a new set of credentials for modification
  *
  * Prepare a new set of task credentials for modification.  A task's creds
  * shouldn't generally be modified directly, therefore this function is used to
  * prepare a new copy, which the caller then modifies and then commits by
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 808060f9effb..089d53978d9e 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -74,39 +74,22 @@ static DEFINE_PER_CPU(struct aa_local_cache, aa_local_buffers);
 static void apparmor_cred_free(struct cred *cred)
 {
 	aa_put_label(cred_label(cred));
 	set_cred_label(cred, NULL);
 }
 
-/*
- * allocate the apparmor part of blank credentials
- */
-static int apparmor_cred_alloc_blank(struct cred *cred, gfp_t gfp)
-{
-	set_cred_label(cred, NULL);
-	return 0;
-}
-
 /*
  * prepare new cred label for modification by prepare_cred block
  */
 static int apparmor_cred_prepare(struct cred *new, const struct cred *old,
 				 gfp_t gfp)
 {
 	set_cred_label(new, aa_get_newest_label(cred_label(old)));
 	return 0;
 }
 
-/*
- * transfer the apparmor data to a blank set of creds
- */
-static void apparmor_cred_transfer(struct cred *new, const struct cred *old)
-{
-	set_cred_label(new, aa_get_newest_label(cred_label(old)));
-}
-
 static void apparmor_task_free(struct task_struct *task)
 {
 
 	aa_free_task_ctx(task_ctx(task));
 }
 
@@ -1504,16 +1487,14 @@ static struct security_hook_list apparmor_hooks[] __ro_after_init = {
 		      apparmor_socket_getpeersec_dgram),
 	LSM_HOOK_INIT(sock_graft, apparmor_sock_graft),
 #ifdef CONFIG_NETWORK_SECMARK
 	LSM_HOOK_INIT(inet_conn_request, apparmor_inet_conn_request),
 #endif
 
-	LSM_HOOK_INIT(cred_alloc_blank, apparmor_cred_alloc_blank),
 	LSM_HOOK_INIT(cred_free, apparmor_cred_free),
 	LSM_HOOK_INIT(cred_prepare, apparmor_cred_prepare),
-	LSM_HOOK_INIT(cred_transfer, apparmor_cred_transfer),
 
 	LSM_HOOK_INIT(bprm_creds_for_exec, apparmor_bprm_creds_for_exec),
 	LSM_HOOK_INIT(bprm_committing_creds, apparmor_bprm_committing_creds),
 	LSM_HOOK_INIT(bprm_committed_creds, apparmor_bprm_committed_creds),
 
 	LSM_HOOK_INIT(task_free, apparmor_task_free),
diff --git a/security/landlock/cred.c b/security/landlock/cred.c
index db9fe7d906ba..786af18c4a1c 100644
--- a/security/landlock/cred.c
+++ b/security/landlock/cred.c
@@ -11,41 +11,34 @@
 
 #include "common.h"
 #include "cred.h"
 #include "ruleset.h"
 #include "setup.h"
 
-static void hook_cred_transfer(struct cred *const new,
-			       const struct cred *const old)
+static int hook_cred_prepare(struct cred *const new,
+			     const struct cred *const old, const gfp_t gfp)
 {
 	struct landlock_ruleset *const old_dom = landlock_cred(old)->domain;
 
 	if (old_dom) {
 		landlock_get_ruleset(old_dom);
 		landlock_cred(new)->domain = old_dom;
 	}
-}
-
-static int hook_cred_prepare(struct cred *const new,
-			     const struct cred *const old, const gfp_t gfp)
-{
-	hook_cred_transfer(new, old);
 	return 0;
 }
 
 static void hook_cred_free(struct cred *const cred)
 {
 	struct landlock_ruleset *const dom = landlock_cred(cred)->domain;
 
 	if (dom)
 		landlock_put_ruleset_deferred(dom);
 }
 
 static struct security_hook_list landlock_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(cred_prepare, hook_cred_prepare),
-	LSM_HOOK_INIT(cred_transfer, hook_cred_transfer),
 	LSM_HOOK_INIT(cred_free, hook_cred_free),
 };
 
 __init void landlock_add_cred_hooks(void)
 {
 	security_add_hooks(landlock_hooks, ARRAY_SIZE(landlock_hooks),
diff --git a/security/security.c b/security/security.c
index 8cee5b6c6e6d..4fb81de5cf80 100644
--- a/security/security.c
+++ b/security/security.c
@@ -3057,35 +3057,12 @@ void security_task_free(struct task_struct *task)
 	call_void_hook(task_free, task);
 
 	kfree(task->security);
 	task->security = NULL;
 }
 
-/**
- * security_cred_alloc_blank() - Allocate the min memory to allow cred_transfer
- * @cred: credentials
- * @gfp: gfp flags
- *
- * Only allocate sufficient memory and attach to @cred such that
- * cred_transfer() will not get ENOMEM.
- *
- * Return: Returns 0 on success, negative values on failure.
- */
-int security_cred_alloc_blank(struct cred *cred, gfp_t gfp)
-{
-	int rc = lsm_cred_alloc(cred, gfp);
-
-	if (rc)
-		return rc;
-
-	rc = call_int_hook(cred_alloc_blank, cred, gfp);
-	if (unlikely(rc))
-		security_cred_free(cred);
-	return rc;
-}
-
 /**
  * security_cred_free() - Free the cred's LSM blob and associated resources
  * @cred: credentials
  *
  * Deallocate and clear the cred->security field in a set of credentials.
  */
@@ -3124,24 +3101,12 @@ int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp)
 	rc = call_int_hook(cred_prepare, new, old, gfp);
 	if (unlikely(rc))
 		security_cred_free(new);
 	return rc;
 }
 
-/**
- * security_transfer_creds() - Transfer creds
- * @new: target credentials
- * @old: original credentials
- *
- * Transfer data from original creds to new creds.
- */
-void security_transfer_creds(struct cred *new, const struct cred *old)
-{
-	call_void_hook(cred_transfer, new, old);
-}
-
 /**
  * security_cred_getsecid() - Get the secid from a set of credentials
  * @c: credentials
  * @secid: secid value
  *
  * Retrieve the security identifier of the cred structure @c.  In case of
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 55c78c318ccd..8a659475cc12 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4007,23 +4007,12 @@ static int selinux_cred_prepare(struct cred *new, const struct cred *old,
 	struct task_security_struct *tsec = selinux_cred(new);
 
 	*tsec = *old_tsec;
 	return 0;
 }
 
-/*
- * transfer the SELinux data to a blank set of creds
- */
-static void selinux_cred_transfer(struct cred *new, const struct cred *old)
-{
-	const struct task_security_struct *old_tsec = selinux_cred(old);
-	struct task_security_struct *tsec = selinux_cred(new);
-
-	*tsec = *old_tsec;
-}
-
 static void selinux_cred_getsecid(const struct cred *c, u32 *secid)
 {
 	*secid = cred_sid(c);
 }
 
 /*
@@ -7213,13 +7202,12 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(file_receive, selinux_file_receive),
 
 	LSM_HOOK_INIT(file_open, selinux_file_open),
 
 	LSM_HOOK_INIT(task_alloc, selinux_task_alloc),
 	LSM_HOOK_INIT(cred_prepare, selinux_cred_prepare),
-	LSM_HOOK_INIT(cred_transfer, selinux_cred_transfer),
 	LSM_HOOK_INIT(cred_getsecid, selinux_cred_getsecid),
 	LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
 	LSM_HOOK_INIT(kernel_create_files_as, selinux_kernel_create_files_as),
 	LSM_HOOK_INIT(kernel_module_request, selinux_kernel_module_request),
 	LSM_HOOK_INIT(kernel_load_data, selinux_kernel_load_data),
 	LSM_HOOK_INIT(kernel_read_file, selinux_kernel_read_file),
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 4164699cd4f6..4cc658deb08b 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2050,27 +2050,12 @@ static int smack_file_open(struct file *file)
 }
 
 /*
  * Task hooks
  */
 
-/**
- * smack_cred_alloc_blank - "allocate" blank task-level security credentials
- * @cred: the new credentials
- * @gfp: the atomicity of any memory allocations
- *
- * Prepare a blank set of credentials for modification.  This must allocate all
- * the memory the LSM module might require such that cred_transfer() can
- * complete without error.
- */
-static int smack_cred_alloc_blank(struct cred *cred, gfp_t gfp)
-{
-	init_task_smack(smack_cred(cred), NULL, NULL);
-	return 0;
-}
-
 
 /**
  * smack_cred_free - "free" task-level security credentials
  * @cred: the credentials in question
  *
  */
@@ -2113,27 +2098,12 @@ static int smack_cred_prepare(struct cred *new, const struct cred *old,
 
 	rc = smk_copy_relabel(&new_tsp->smk_relabel, &old_tsp->smk_relabel,
 				gfp);
 	return rc;
 }
 
-/**
- * smack_cred_transfer - Transfer the old credentials to the new credentials
- * @new: the new credentials
- * @old: the original credentials
- *
- * Fill in a set of blank credentials from another set of credentials.
- */
-static void smack_cred_transfer(struct cred *new, const struct cred *old)
-{
-	struct task_smack *old_tsp = smack_cred(old);
-	struct task_smack *new_tsp = smack_cred(new);
-
-	init_task_smack(new_tsp, old_tsp->smk_task, old_tsp->smk_task);
-}
-
 /**
  * smack_cred_getsecid - get the secid corresponding to a creds structure
  * @cred: the object creds
  * @secid: where to put the result
  *
  * Sets the secid to contain a u32 version of the smack label.
@@ -5107,16 +5077,14 @@ static struct security_hook_list smack_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(file_set_fowner, smack_file_set_fowner),
 	LSM_HOOK_INIT(file_send_sigiotask, smack_file_send_sigiotask),
 	LSM_HOOK_INIT(file_receive, smack_file_receive),
 
 	LSM_HOOK_INIT(file_open, smack_file_open),
 
-	LSM_HOOK_INIT(cred_alloc_blank, smack_cred_alloc_blank),
 	LSM_HOOK_INIT(cred_free, smack_cred_free),
 	LSM_HOOK_INIT(cred_prepare, smack_cred_prepare),
-	LSM_HOOK_INIT(cred_transfer, smack_cred_transfer),
 	LSM_HOOK_INIT(cred_getsecid, smack_cred_getsecid),
 	LSM_HOOK_INIT(kernel_act_as, smack_kernel_act_as),
 	LSM_HOOK_INIT(kernel_create_files_as, smack_kernel_create_files_as),
 	LSM_HOOK_INIT(task_setpgid, smack_task_setpgid),
 	LSM_HOOK_INIT(task_getpgid, smack_task_getpgid),
 	LSM_HOOK_INIT(task_getsid, smack_task_getsid),

-- 
2.46.0.rc2.264.g509ed76dc8-goog

Re: [PATCH v2 1/2] KEYS: use synchronous task work for changing parent credentials

[Jarkko Sakkinen]

On Mon Aug 5, 2024 at 2:54 PM EEST, Jann Horn wrote:
> keyctl_session_to_parent() involves posting task work to the parent task,
> with work function key_change_session_keyring.
> Because the task work in the parent runs asynchronously, no errors can be
> returned back to the caller of keyctl_session_to_parent(), and therefore
> the work function key_change_session_keyring() can't be allowed to fail due
> to things like memory allocation failure or permission checks - all
> allocations and checks have to happen in the child.
>
> This is annoying for two reasons:
>
>  - It is the only reason why cred_alloc_blank() and
>    security_transfer_creds() are necessary.
>  - It means we can't do synchronous permission checks.

I agree with this premise. Also I think the code change is reasonable.

I'd like to see a comment from David tho.

BR, Jarkko

Re: [PATCH v2 2/2] security: remove unused cred_alloc_blank/cred_transfer helpers

[Jarkko Sakkinen]

On Mon Aug 5, 2024 at 2:54 PM EEST, Jann Horn wrote:
> Now that they're unused thanks to the preceding commit, remove
> cred_alloc_blank(), security_transfer_creds(), and the corresponding LSM
> hook implementations.
>
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
>  include/linux/cred.h          |  1 -
>  include/linux/lsm_hook_defs.h |  3 ---
>  include/linux/security.h      | 12 ------------
>  kernel/cred.c                 | 23 -----------------------
>  security/apparmor/lsm.c       | 19 -------------------
>  security/landlock/cred.c      | 11 ++---------
>  security/security.c           | 35 -----------------------------------
>  security/selinux/hooks.c      | 12 ------------
>  security/smack/smack_lsm.c    | 32 --------------------------------
>  9 files changed, 2 insertions(+), 146 deletions(-)
>
> diff --git a/include/linux/cred.h b/include/linux/cred.h
> index 2976f534a7a3..54b5105d4cd5 100644
> --- a/include/linux/cred.h
> +++ b/include/linux/cred.h
> @@ -147,13 +147,12 @@ struct cred {
>  } __randomize_layout;
>  
>  extern void __put_cred(struct cred *);
>  extern void exit_creds(struct task_struct *);
>  extern int copy_creds(struct task_struct *, unsigned long);
>  extern const struct cred *get_task_cred(struct task_struct *);
> -extern struct cred *cred_alloc_blank(void);
>  extern struct cred *prepare_creds(void);
>  extern struct cred *prepare_exec_creds(void);
>  extern int commit_creds(struct cred *);
>  extern void abort_creds(struct cred *);
>  extern const struct cred *override_creds(const struct cred *);
>  extern void revert_creds(const struct cred *);
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 855db460e08b..1d75075cb607 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -204,18 +204,15 @@ LSM_HOOK(int, 0, file_receive, struct file *file)
>  LSM_HOOK(int, 0, file_open, struct file *file)
>  LSM_HOOK(int, 0, file_post_open, struct file *file, int mask)
>  LSM_HOOK(int, 0, file_truncate, struct file *file)
>  LSM_HOOK(int, 0, task_alloc, struct task_struct *task,
>  	 unsigned long clone_flags)
>  LSM_HOOK(void, LSM_RET_VOID, task_free, struct task_struct *task)
> -LSM_HOOK(int, 0, cred_alloc_blank, struct cred *cred, gfp_t gfp)
>  LSM_HOOK(void, LSM_RET_VOID, cred_free, struct cred *cred)
>  LSM_HOOK(int, 0, cred_prepare, struct cred *new, const struct cred *old,
>  	 gfp_t gfp)
> -LSM_HOOK(void, LSM_RET_VOID, cred_transfer, struct cred *new,
> -	 const struct cred *old)
>  LSM_HOOK(void, LSM_RET_VOID, cred_getsecid, const struct cred *c, u32 *secid)
>  LSM_HOOK(int, 0, kernel_act_as, struct cred *new, u32 secid)
>  LSM_HOOK(int, 0, kernel_create_files_as, struct cred *new, struct inode *inode)
>  LSM_HOOK(int, 0, kernel_module_request, char *kmod_name)
>  LSM_HOOK(int, 0, kernel_load_data, enum kernel_load_data_id id, bool contents)
>  LSM_HOOK(int, 0, kernel_post_load_data, char *buf, loff_t size,
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 1390f1efb4f0..a366c2a03f55 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -421,16 +421,14 @@ int security_file_send_sigiotask(struct task_struct *tsk,
>  int security_file_receive(struct file *file);
>  int security_file_open(struct file *file);
>  int security_file_post_open(struct file *file, int mask);
>  int security_file_truncate(struct file *file);
>  int security_task_alloc(struct task_struct *task, unsigned long clone_flags);
>  void security_task_free(struct task_struct *task);
> -int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
>  void security_cred_free(struct cred *cred);
>  int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp);
> -void security_transfer_creds(struct cred *new, const struct cred *old);
>  void security_cred_getsecid(const struct cred *c, u32 *secid);
>  int security_kernel_act_as(struct cred *new, u32 secid);
>  int security_kernel_create_files_as(struct cred *new, struct inode *inode);
>  int security_kernel_module_request(char *kmod_name);
>  int security_kernel_load_data(enum kernel_load_data_id id, bool contents);
>  int security_kernel_post_load_data(char *buf, loff_t size,
> @@ -1117,32 +1115,22 @@ static inline int security_task_alloc(struct task_struct *task,
>  	return 0;
>  }
>  
>  static inline void security_task_free(struct task_struct *task)
>  { }
>  
> -static inline int security_cred_alloc_blank(struct cred *cred, gfp_t gfp)
> -{
> -	return 0;
> -}
> -
>  static inline void security_cred_free(struct cred *cred)
>  { }
>  
>  static inline int security_prepare_creds(struct cred *new,
>  					 const struct cred *old,
>  					 gfp_t gfp)
>  {
>  	return 0;
>  }
>  
> -static inline void security_transfer_creds(struct cred *new,
> -					   const struct cred *old)
> -{
> -}
> -
>  static inline void security_cred_getsecid(const struct cred *c, u32 *secid)
>  {
>  	*secid = 0;
>  }
>  
>  static inline int security_kernel_act_as(struct cred *cred, u32 secid)
> diff --git a/kernel/cred.c b/kernel/cred.c
> index 075cfa7c896f..b2f6130cd6b7 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -163,35 +163,12 @@ const struct cred *get_task_cred(struct task_struct *task)
>  
>  	rcu_read_unlock();
>  	return cred;
>  }
>  EXPORT_SYMBOL(get_task_cred);
>  
> -/*
> - * Allocate blank credentials, such that the credentials can be filled in at a
> - * later date without risk of ENOMEM.
> - */
> -struct cred *cred_alloc_blank(void)
> -{
> -	struct cred *new;
> -
> -	new = kmem_cache_zalloc(cred_jar, GFP_KERNEL);
> -	if (!new)
> -		return NULL;
> -
> -	atomic_long_set(&new->usage, 1);
> -	if (security_cred_alloc_blank(new, GFP_KERNEL_ACCOUNT) < 0)
> -		goto error;
> -
> -	return new;
> -
> -error:
> -	abort_creds(new);
> -	return NULL;
> -}
> -
>  /**
>   * prepare_creds - Prepare a new set of credentials for modification
>   *
>   * Prepare a new set of task credentials for modification.  A task's creds
>   * shouldn't generally be modified directly, therefore this function is used to
>   * prepare a new copy, which the caller then modifies and then commits by
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 808060f9effb..089d53978d9e 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -74,39 +74,22 @@ static DEFINE_PER_CPU(struct aa_local_cache, aa_local_buffers);
>  static void apparmor_cred_free(struct cred *cred)
>  {
>  	aa_put_label(cred_label(cred));
>  	set_cred_label(cred, NULL);
>  }
>  
> -/*
> - * allocate the apparmor part of blank credentials
> - */
> -static int apparmor_cred_alloc_blank(struct cred *cred, gfp_t gfp)
> -{
> -	set_cred_label(cred, NULL);
> -	return 0;
> -}
> -
>  /*
>   * prepare new cred label for modification by prepare_cred block
>   */
>  static int apparmor_cred_prepare(struct cred *new, const struct cred *old,
>  				 gfp_t gfp)
>  {
>  	set_cred_label(new, aa_get_newest_label(cred_label(old)));
>  	return 0;
>  }
>  
> -/*
> - * transfer the apparmor data to a blank set of creds
> - */
> -static void apparmor_cred_transfer(struct cred *new, const struct cred *old)
> -{
> -	set_cred_label(new, aa_get_newest_label(cred_label(old)));
> -}
> -
>  static void apparmor_task_free(struct task_struct *task)
>  {
>  
>  	aa_free_task_ctx(task_ctx(task));
>  }
>  
> @@ -1504,16 +1487,14 @@ static struct security_hook_list apparmor_hooks[] __ro_after_init = {
>  		      apparmor_socket_getpeersec_dgram),
>  	LSM_HOOK_INIT(sock_graft, apparmor_sock_graft),
>  #ifdef CONFIG_NETWORK_SECMARK
>  	LSM_HOOK_INIT(inet_conn_request, apparmor_inet_conn_request),
>  #endif
>  
> -	LSM_HOOK_INIT(cred_alloc_blank, apparmor_cred_alloc_blank),
>  	LSM_HOOK_INIT(cred_free, apparmor_cred_free),
>  	LSM_HOOK_INIT(cred_prepare, apparmor_cred_prepare),
> -	LSM_HOOK_INIT(cred_transfer, apparmor_cred_transfer),
>  
>  	LSM_HOOK_INIT(bprm_creds_for_exec, apparmor_bprm_creds_for_exec),
>  	LSM_HOOK_INIT(bprm_committing_creds, apparmor_bprm_committing_creds),
>  	LSM_HOOK_INIT(bprm_committed_creds, apparmor_bprm_committed_creds),
>  
>  	LSM_HOOK_INIT(task_free, apparmor_task_free),
> diff --git a/security/landlock/cred.c b/security/landlock/cred.c
> index db9fe7d906ba..786af18c4a1c 100644
> --- a/security/landlock/cred.c
> +++ b/security/landlock/cred.c
> @@ -11,41 +11,34 @@
>  
>  #include "common.h"
>  #include "cred.h"
>  #include "ruleset.h"
>  #include "setup.h"
>  
> -static void hook_cred_transfer(struct cred *const new,
> -			       const struct cred *const old)
> +static int hook_cred_prepare(struct cred *const new,
> +			     const struct cred *const old, const gfp_t gfp)
>  {
>  	struct landlock_ruleset *const old_dom = landlock_cred(old)->domain;
>  
>  	if (old_dom) {
>  		landlock_get_ruleset(old_dom);
>  		landlock_cred(new)->domain = old_dom;
>  	}
> -}
> -
> -static int hook_cred_prepare(struct cred *const new,
> -			     const struct cred *const old, const gfp_t gfp)
> -{
> -	hook_cred_transfer(new, old);
>  	return 0;
>  }
>  
>  static void hook_cred_free(struct cred *const cred)
>  {
>  	struct landlock_ruleset *const dom = landlock_cred(cred)->domain;
>  
>  	if (dom)
>  		landlock_put_ruleset_deferred(dom);
>  }
>  
>  static struct security_hook_list landlock_hooks[] __ro_after_init = {
>  	LSM_HOOK_INIT(cred_prepare, hook_cred_prepare),
> -	LSM_HOOK_INIT(cred_transfer, hook_cred_transfer),
>  	LSM_HOOK_INIT(cred_free, hook_cred_free),
>  };
>  
>  __init void landlock_add_cred_hooks(void)
>  {
>  	security_add_hooks(landlock_hooks, ARRAY_SIZE(landlock_hooks),
> diff --git a/security/security.c b/security/security.c
> index 8cee5b6c6e6d..4fb81de5cf80 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -3057,35 +3057,12 @@ void security_task_free(struct task_struct *task)
>  	call_void_hook(task_free, task);
>  
>  	kfree(task->security);
>  	task->security = NULL;
>  }
>  
> -/**
> - * security_cred_alloc_blank() - Allocate the min memory to allow cred_transfer
> - * @cred: credentials
> - * @gfp: gfp flags
> - *
> - * Only allocate sufficient memory and attach to @cred such that
> - * cred_transfer() will not get ENOMEM.
> - *
> - * Return: Returns 0 on success, negative values on failure.
> - */
> -int security_cred_alloc_blank(struct cred *cred, gfp_t gfp)
> -{
> -	int rc = lsm_cred_alloc(cred, gfp);
> -
> -	if (rc)
> -		return rc;
> -
> -	rc = call_int_hook(cred_alloc_blank, cred, gfp);
> -	if (unlikely(rc))
> -		security_cred_free(cred);
> -	return rc;
> -}
> -
>  /**
>   * security_cred_free() - Free the cred's LSM blob and associated resources
>   * @cred: credentials
>   *
>   * Deallocate and clear the cred->security field in a set of credentials.
>   */
> @@ -3124,24 +3101,12 @@ int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp)
>  	rc = call_int_hook(cred_prepare, new, old, gfp);
>  	if (unlikely(rc))
>  		security_cred_free(new);
>  	return rc;
>  }
>  
> -/**
> - * security_transfer_creds() - Transfer creds
> - * @new: target credentials
> - * @old: original credentials
> - *
> - * Transfer data from original creds to new creds.
> - */
> -void security_transfer_creds(struct cred *new, const struct cred *old)
> -{
> -	call_void_hook(cred_transfer, new, old);
> -}
> -
>  /**
>   * security_cred_getsecid() - Get the secid from a set of credentials
>   * @c: credentials
>   * @secid: secid value
>   *
>   * Retrieve the security identifier of the cred structure @c.  In case of
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 55c78c318ccd..8a659475cc12 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4007,23 +4007,12 @@ static int selinux_cred_prepare(struct cred *new, const struct cred *old,
>  	struct task_security_struct *tsec = selinux_cred(new);
>  
>  	*tsec = *old_tsec;
>  	return 0;
>  }
>  
> -/*
> - * transfer the SELinux data to a blank set of creds
> - */
> -static void selinux_cred_transfer(struct cred *new, const struct cred *old)
> -{
> -	const struct task_security_struct *old_tsec = selinux_cred(old);
> -	struct task_security_struct *tsec = selinux_cred(new);
> -
> -	*tsec = *old_tsec;
> -}
> -
>  static void selinux_cred_getsecid(const struct cred *c, u32 *secid)
>  {
>  	*secid = cred_sid(c);
>  }
>  
>  /*
> @@ -7213,13 +7202,12 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
>  	LSM_HOOK_INIT(file_receive, selinux_file_receive),
>  
>  	LSM_HOOK_INIT(file_open, selinux_file_open),
>  
>  	LSM_HOOK_INIT(task_alloc, selinux_task_alloc),
>  	LSM_HOOK_INIT(cred_prepare, selinux_cred_prepare),
> -	LSM_HOOK_INIT(cred_transfer, selinux_cred_transfer),
>  	LSM_HOOK_INIT(cred_getsecid, selinux_cred_getsecid),
>  	LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
>  	LSM_HOOK_INIT(kernel_create_files_as, selinux_kernel_create_files_as),
>  	LSM_HOOK_INIT(kernel_module_request, selinux_kernel_module_request),
>  	LSM_HOOK_INIT(kernel_load_data, selinux_kernel_load_data),
>  	LSM_HOOK_INIT(kernel_read_file, selinux_kernel_read_file),
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 4164699cd4f6..4cc658deb08b 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -2050,27 +2050,12 @@ static int smack_file_open(struct file *file)
>  }
>  
>  /*
>   * Task hooks
>   */
>  
> -/**
> - * smack_cred_alloc_blank - "allocate" blank task-level security credentials
> - * @cred: the new credentials
> - * @gfp: the atomicity of any memory allocations
> - *
> - * Prepare a blank set of credentials for modification.  This must allocate all
> - * the memory the LSM module might require such that cred_transfer() can
> - * complete without error.
> - */
> -static int smack_cred_alloc_blank(struct cred *cred, gfp_t gfp)
> -{
> -	init_task_smack(smack_cred(cred), NULL, NULL);
> -	return 0;
> -}
> -
>  
>  /**
>   * smack_cred_free - "free" task-level security credentials
>   * @cred: the credentials in question
>   *
>   */
> @@ -2113,27 +2098,12 @@ static int smack_cred_prepare(struct cred *new, const struct cred *old,
>  
>  	rc = smk_copy_relabel(&new_tsp->smk_relabel, &old_tsp->smk_relabel,
>  				gfp);
>  	return rc;
>  }
>  
> -/**
> - * smack_cred_transfer - Transfer the old credentials to the new credentials
> - * @new: the new credentials
> - * @old: the original credentials
> - *
> - * Fill in a set of blank credentials from another set of credentials.
> - */
> -static void smack_cred_transfer(struct cred *new, const struct cred *old)
> -{
> -	struct task_smack *old_tsp = smack_cred(old);
> -	struct task_smack *new_tsp = smack_cred(new);
> -
> -	init_task_smack(new_tsp, old_tsp->smk_task, old_tsp->smk_task);
> -}
> -
>  /**
>   * smack_cred_getsecid - get the secid corresponding to a creds structure
>   * @cred: the object creds
>   * @secid: where to put the result
>   *
>   * Sets the secid to contain a u32 version of the smack label.
> @@ -5107,16 +5077,14 @@ static struct security_hook_list smack_hooks[] __ro_after_init = {
>  	LSM_HOOK_INIT(file_set_fowner, smack_file_set_fowner),
>  	LSM_HOOK_INIT(file_send_sigiotask, smack_file_send_sigiotask),
>  	LSM_HOOK_INIT(file_receive, smack_file_receive),
>  
>  	LSM_HOOK_INIT(file_open, smack_file_open),
>  
> -	LSM_HOOK_INIT(cred_alloc_blank, smack_cred_alloc_blank),
>  	LSM_HOOK_INIT(cred_free, smack_cred_free),
>  	LSM_HOOK_INIT(cred_prepare, smack_cred_prepare),
> -	LSM_HOOK_INIT(cred_transfer, smack_cred_transfer),
>  	LSM_HOOK_INIT(cred_getsecid, smack_cred_getsecid),
>  	LSM_HOOK_INIT(kernel_act_as, smack_kernel_act_as),
>  	LSM_HOOK_INIT(kernel_create_files_as, smack_kernel_create_files_as),
>  	LSM_HOOK_INIT(task_setpgid, smack_task_setpgid),
>  	LSM_HOOK_INIT(task_getpgid, smack_task_getpgid),
>  	LSM_HOOK_INIT(task_getsid, smack_task_getsid),

I'll give this reviewd-by as soon as getting some feedback David
on 1/2. I mean this is dead obvious with a condition.

BR, Jarkko

Can KEYCTL_SESSION_TO_PARENT be dropped entirely? -- was Re: [PATCH v2 1/2] KEYS: use synchronous task work for changing parent credentials

[David Howells]

Jann Horn <jannh@google.com> wrote:

> Rewrite keyctl_session_to_parent() to run task work on the parent
> synchronously, so that any errors that happen in the task work can be
> plumbed back into the syscall return value in the child.

The main thing I worry about is if there's a way to deadlock the child and the
parent against each other.  vfork() for example.

> +	if (task_work_cancel(parent, &ctx.work)) {
> +		/*
> +		 * We got interrupted and the task work was canceled before it
> +		 * could execute.
> +		 * Use -ERESTARTNOINTR instead of -ERESTARTSYS for
> +		 * compatibility - the manpage does not list -EINTR as a
> +		 * possible error for keyctl().
> +		 */

I think returning EINTR is fine, provided that if we return EINTR, the change
didn't happen.  KEYCTL_SESSION_TO_PARENT is only used by the aklog, dlog and
klog* OpenAFS programs AFAIK, and only if "-setpag" is set as a command line
option.  It also won't be effective if you strace the program.

Maybe the AFS people can say whether it's even worth keeping the functionality
rather than just dropping KEYCTL_SESSION_TO_PARENT?

David

Re: Can KEYCTL_SESSION_TO_PARENT be dropped entirely? -- was Re: [PATCH v2 1/2] KEYS: use synchronous task work for changing parent credentials

[Jann Horn]

On Thu, Aug 15, 2024 at 9:46 PM David Howells <dhowells@redhat.com> wrote:
> Jann Horn <jannh@google.com> wrote:
>
> > Rewrite keyctl_session_to_parent() to run task work on the parent
> > synchronously, so that any errors that happen in the task work can be
> > plumbed back into the syscall return value in the child.
>
> The main thing I worry about is if there's a way to deadlock the child and the
> parent against each other.  vfork() for example.

Yes - I think it would work fine for scenarios like using
KEYCTL_SESSION_TO_PARENT from a helper binary against the shell that
launched the helper (which I think is the intended usecase?), but
there could theoretically be constellations where it would cause an
(interruptible) hang if the parent is stuck in
uninterruptible/killable sleep.

I think vfork() is rather special in that it does a killable wait for
the child to exit or execute; and based on my understanding of the
intended usecase of KEYCTL_SESSION_TO_PARENT, I think normally
KEYCTL_SESSION_TO_PARENT would only be used by a child that has gone
through execve?


> > +     if (task_work_cancel(parent, &ctx.work)) {
> > +             /*
> > +              * We got interrupted and the task work was canceled before it
> > +              * could execute.
> > +              * Use -ERESTARTNOINTR instead of -ERESTARTSYS for
> > +              * compatibility - the manpage does not list -EINTR as a
> > +              * possible error for keyctl().
> > +              */
>
> I think returning EINTR is fine, provided that if we return EINTR, the change
> didn't happen.  KEYCTL_SESSION_TO_PARENT is only used by the aklog, dlog and
> klog* OpenAFS programs AFAIK, and only if "-setpag" is set as a command line
> option.  It also won't be effective if you strace the program.

Ah, I didn't even know about those.

The users I knew of are the command-line tools "keyctl new_session"
and "e4crypt new_session" (see
https://codesearch.debian.net/search?q=KEYCTL_SESSION_TO_PARENT&literal=1,
which indexes code that's part of Debian).

> Maybe the AFS people can say whether it's even worth keeping the functionality
> rather than just dropping KEYCTL_SESSION_TO_PARENT?

I think this would break the tools "keyctl new_session" and "e4crypt
new_session" - though I don't know if anyone actually uses those
invocations.

Re: Can KEYCTL_SESSION_TO_PARENT be dropped entirely? -- was Re: [PATCH v2 1/2] KEYS: use synchronous task work for changing parent credentials

[Jarkko Sakkinen]

On Thu Aug 15, 2024 at 10:59 PM EEST, Jann Horn wrote:
> On Thu, Aug 15, 2024 at 9:46 PM David Howells <dhowells@redhat.com> wrote:
> > Jann Horn <jannh@google.com> wrote:
> >
> > > Rewrite keyctl_session_to_parent() to run task work on the parent
> > > synchronously, so that any errors that happen in the task work can be
> > > plumbed back into the syscall return value in the child.
> >
> > The main thing I worry about is if there's a way to deadlock the child and the
> > parent against each other.  vfork() for example.
>
> Yes - I think it would work fine for scenarios like using
> KEYCTL_SESSION_TO_PARENT from a helper binary against the shell that
> launched the helper (which I think is the intended usecase?), but
> there could theoretically be constellations where it would cause an
> (interruptible) hang if the parent is stuck in
> uninterruptible/killable sleep.
>
> I think vfork() is rather special in that it does a killable wait for
> the child to exit or execute; and based on my understanding of the
> intended usecase of KEYCTL_SESSION_TO_PARENT, I think normally
> KEYCTL_SESSION_TO_PARENT would only be used by a child that has gone
> through execve?

Could this encapsulated to a kselftest? Like having host process
that forks the payload and send SIGINT. That could be deployed e.g
to tools/testing/selftests/keys. Would be nice to be able to try
this out with a low barrier.

Doing this type of testing is different axis than keyutils test suite
IMHO. That would be also great starting point for adding concurrency
tests in future.

Could be done either in C or Python.

BR, Jarkko

Re: Can KEYCTL_SESSION_TO_PARENT be dropped entirely? -- was Re: [PATCH v2 1/2] KEYS: use synchronous task work for changing parent credentials

[Paul Moore]

On Thu, Aug 15, 2024 at 4:00 PM Jann Horn <jannh@google.com> wrote:
> On Thu, Aug 15, 2024 at 9:46 PM David Howells <dhowells@redhat.com> wrote:
> > Jann Horn <jannh@google.com> wrote:
> >
> > > Rewrite keyctl_session_to_parent() to run task work on the parent
> > > synchronously, so that any errors that happen in the task work can be
> > > plumbed back into the syscall return value in the child.
> >
> > The main thing I worry about is if there's a way to deadlock the child and the
> > parent against each other.  vfork() for example.
>
> Yes - I think it would work fine for scenarios like using
> KEYCTL_SESSION_TO_PARENT from a helper binary against the shell that
> launched the helper (which I think is the intended usecase?), but
> there could theoretically be constellations where it would cause an
> (interruptible) hang if the parent is stuck in
> uninterruptible/killable sleep.
>
> I think vfork() is rather special in that it does a killable wait for
> the child to exit or execute; and based on my understanding of the
> intended usecase of KEYCTL_SESSION_TO_PARENT, I think normally
> KEYCTL_SESSION_TO_PARENT would only be used by a child that has gone
> through execve?

Where did we land on all of this?  Unless I missed a thread somewhere,
it looks like the discussion trailed off without any resolution on if
we are okay with a potentially (interruptible) deadlock?

-- 
paul-moore.com

Re: [PATCH v2 1/2] KEYS: use synchronous task work for changing parent credentials

[Paul Moore]

On Aug  5, 2024 Jann Horn <jannh@google.com> wrote:
> 
> keyctl_session_to_parent() involves posting task work to the parent task,
> with work function key_change_session_keyring.
> Because the task work in the parent runs asynchronously, no errors can be
> returned back to the caller of keyctl_session_to_parent(), and therefore
> the work function key_change_session_keyring() can't be allowed to fail due
> to things like memory allocation failure or permission checks - all
> allocations and checks have to happen in the child.
> 
> This is annoying for two reasons:
> 
>  - It is the only reason why cred_alloc_blank() and
>    security_transfer_creds() are necessary.
>  - It means we can't do synchronous permission checks.
> 
> Rewrite keyctl_session_to_parent() to run task work on the parent
> synchronously, so that any errors that happen in the task work can be
> plumbed back into the syscall return value in the child.
> This allows us to get rid of cred_alloc_blank() and
> security_transfer_creds() in a later commit, and it will make it possible
> to write more reliable security checks for this operation.
> 
> Note that this requires using TWA_SIGNAL instead of TWA_RESUME, so the
> parent might observe some spurious -EAGAIN syscall returns or such; but the
> parent likely anyway has to be ready to deal with the side effects of
> receiving signals (since it'll probably get SIGCHLD when the child dies),
> so that probably isn't an issue.
> 
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
>  security/keys/internal.h     |   8 ++++
>  security/keys/keyctl.c       | 107 +++++++++++++------------------------------
>  security/keys/process_keys.c |  86 ++++++++++++++++++----------------
>  3 files changed, 87 insertions(+), 114 deletions(-)

...

> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index ab927a142f51..e4cfe5c4594a 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -1616,104 +1616,63 @@ long keyctl_get_security(key_serial_t keyid,
>   * parent process.
>   *
>   * The keyring must exist and must grant the caller LINK permission, and the
>   * parent process must be single-threaded and must have the same effective
>   * ownership as this process and mustn't be SUID/SGID.
>   *
> - * The keyring will be emplaced on the parent when it next resumes userspace.
> + * The keyring will be emplaced on the parent via a pseudo-signal.
>   *
>   * If successful, 0 will be returned.
>   */
>  long keyctl_session_to_parent(void)
>  {
> -	struct task_struct *me, *parent;
> -	const struct cred *mycred, *pcred;
> -	struct callback_head *newwork, *oldwork;
> +	struct keyctl_session_to_parent_context ctx;
> +	struct task_struct *parent;
>  	key_ref_t keyring_r;
> -	struct cred *cred;
>  	int ret;
>  
>  	keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING, 0, KEY_NEED_LINK);
>  	if (IS_ERR(keyring_r))
>  		return PTR_ERR(keyring_r);
>  
> -	ret = -ENOMEM;
> -
> -	/* our parent is going to need a new cred struct, a new tgcred struct
> -	 * and new security data, so we allocate them here to prevent ENOMEM in
> -	 * our parent */
> -	cred = cred_alloc_blank();
> -	if (!cred)
> -		goto error_keyring;
> -	newwork = &cred->rcu;
> +	write_lock_irq(&tasklist_lock);
> +	parent = get_task_struct(rcu_dereference_protected(current->real_parent,
> +					lockdep_is_held(&tasklist_lock)));
> +	write_unlock_irq(&tasklist_lock);
>  
> -	cred->session_keyring = key_ref_to_ptr(keyring_r);
> -	keyring_r = NULL;
> -	init_task_work(newwork, key_change_session_keyring);
> +	/* the parent mustn't be init and mustn't be a kernel thread */
> +	if (is_global_init(parent) || (READ_ONCE(parent->flags) & PF_KTHREAD) != 0)
> +		goto put_task;

I think we need to explicitly set @ret if we are failing here, yes?
  
> -	me = current;
> -	rcu_read_lock();
> -	write_lock_irq(&tasklist_lock);
> +	ctx.new_session_keyring = key_ref_to_ptr(keyring_r);
> +	ctx.child_cred = current_cred();
> +	init_completion(&ctx.done);
> +	init_task_work(&ctx.work, key_change_session_keyring);
> +	ret = task_work_add(parent, &ctx.work, TWA_SIGNAL);
> +	if (ret)
> +		goto put_task;
>  
> -	ret = -EPERM;
> -	oldwork = NULL;
> -	parent = rcu_dereference_protected(me->real_parent,
> -					   lockdep_is_held(&tasklist_lock));
> +	ret = wait_for_completion_interruptible(&ctx.done);
>  
> -	/* the parent mustn't be init and mustn't be a kernel thread */
> -	if (parent->pid <= 1 || !parent->mm)
> -		goto unlock;
> -
> -	/* the parent must be single threaded */
> -	if (!thread_group_empty(parent))
> -		goto unlock;
> -
> -	/* the parent and the child must have different session keyrings or
> -	 * there's no point */
> -	mycred = current_cred();
> -	pcred = __task_cred(parent);
> -	if (mycred == pcred ||
> -	    mycred->session_keyring == pcred->session_keyring) {
> -		ret = 0;
> -		goto unlock;
> +	if (task_work_cancel(parent, &ctx.work)) {
> +		/*
> +		 * We got interrupted and the task work was canceled before it
> +		 * could execute.
> +		 * Use -ERESTARTNOINTR instead of -ERESTARTSYS for
> +		 * compatibility - the manpage does not list -EINTR as a
> +		 * possible error for keyctl().
> +		 */
> +		ret = -ERESTARTNOINTR;
> +	} else {
> +		/* task work is running or has been executed */
> +		wait_for_completion(&ctx.done);
> +		ret = ctx.result;
>  	}
>  
> -	/* the parent must have the same effective ownership and mustn't be
> -	 * SUID/SGID */
> -	if (!uid_eq(pcred->uid,	 mycred->euid) ||
> -	    !uid_eq(pcred->euid, mycred->euid) ||
> -	    !uid_eq(pcred->suid, mycred->euid) ||
> -	    !gid_eq(pcred->gid,	 mycred->egid) ||
> -	    !gid_eq(pcred->egid, mycred->egid) ||
> -	    !gid_eq(pcred->sgid, mycred->egid))
> -		goto unlock;
> -
> -	/* the keyrings must have the same UID */
> -	if ((pcred->session_keyring &&
> -	     !uid_eq(pcred->session_keyring->uid, mycred->euid)) ||
> -	    !uid_eq(mycred->session_keyring->uid, mycred->euid))
> -		goto unlock;
> -
> -	/* cancel an already pending keyring replacement */
> -	oldwork = task_work_cancel_func(parent, key_change_session_keyring);
> -
> -	/* the replacement session keyring is applied just prior to userspace
> -	 * restarting */
> -	ret = task_work_add(parent, newwork, TWA_RESUME);
> -	if (!ret)
> -		newwork = NULL;
> -unlock:
> -	write_unlock_irq(&tasklist_lock);
> -	rcu_read_unlock();
> -	if (oldwork)
> -		put_cred(container_of(oldwork, struct cred, rcu));
> -	if (newwork)
> -		put_cred(cred);
> -	return ret;
> -
> -error_keyring:
> +put_task:
> +	put_task_struct(parent);
>  	key_ref_put(keyring_r);
>  	return ret;
>  }

--
paul-moore.com

Re: [PATCH v2 1/2] KEYS: use synchronous task work for changing parent credentials

[Jann Horn]

On Tue, Sep 10, 2024 at 11:07 PM Paul Moore <paul@paul-moore.com> wrote:
> On Aug  5, 2024 Jann Horn <jannh@google.com> wrote:
> > -     cred->session_keyring = key_ref_to_ptr(keyring_r);
> > -     keyring_r = NULL;
> > -     init_task_work(newwork, key_change_session_keyring);
> > +     /* the parent mustn't be init and mustn't be a kernel thread */
> > +     if (is_global_init(parent) || (READ_ONCE(parent->flags) & PF_KTHREAD) != 0)
> > +             goto put_task;
>
> I think we need to explicitly set @ret if we are failing here, yes?

Ah, yes. Thanks.

Re: Can KEYCTL_SESSION_TO_PARENT be dropped entirely? -- was Re: [PATCH v2 1/2] KEYS: use synchronous task work for changing parent credentials

[Paul Moore]

On Tue, Sep 10, 2024 at 4:49 PM Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Aug 15, 2024 at 4:00 PM Jann Horn <jannh@google.com> wrote:
> > On Thu, Aug 15, 2024 at 9:46 PM David Howells <dhowells@redhat.com> wrote:
> > > Jann Horn <jannh@google.com> wrote:
> > >
> > > > Rewrite keyctl_session_to_parent() to run task work on the parent
> > > > synchronously, so that any errors that happen in the task work can be
> > > > plumbed back into the syscall return value in the child.
> > >
> > > The main thing I worry about is if there's a way to deadlock the child and the
> > > parent against each other.  vfork() for example.
> >
> > Yes - I think it would work fine for scenarios like using
> > KEYCTL_SESSION_TO_PARENT from a helper binary against the shell that
> > launched the helper (which I think is the intended usecase?), but
> > there could theoretically be constellations where it would cause an
> > (interruptible) hang if the parent is stuck in
> > uninterruptible/killable sleep.
> >
> > I think vfork() is rather special in that it does a killable wait for
> > the child to exit or execute; and based on my understanding of the
> > intended usecase of KEYCTL_SESSION_TO_PARENT, I think normally
> > KEYCTL_SESSION_TO_PARENT would only be used by a child that has gone
> > through execve?
>
> Where did we land on all of this?  Unless I missed a thread somewhere,
> it looks like the discussion trailed off without any resolution on if
> we are okay with a potentially (interruptible) deadlock?

As a potential tweak to this, what if we gave up on the idea of
returning the error code so we could avoid the signal deadlock issue?
I suppose there could be an issue if the parent was
expecting/depending on keyring change from the child, but honestly, if
the parent is relying on the kernel keyring and spawning a child
process without restring the KEYCTL_SESSION_TO_PARENT then the parent
really should be doing some sanity checks on the keyring after the
child returns anyway.

I'm conflicted on the best way to solve this problem, but I think we
need to fix this somehow as I believe the current behavior is broken
...

-- 
paul-moore.com

Re: Can KEYCTL_SESSION_TO_PARENT be dropped entirely? -- was Re: [PATCH v2 1/2] KEYS: use synchronous task work for changing parent credentials

[Jann Horn]

On Mon, Sep 16, 2024 at 12:46 PM Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Sep 10, 2024 at 4:49 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Thu, Aug 15, 2024 at 4:00 PM Jann Horn <jannh@google.com> wrote:
> > > On Thu, Aug 15, 2024 at 9:46 PM David Howells <dhowells@redhat.com> wrote:
> > > > Jann Horn <jannh@google.com> wrote:
> > > >
> > > > > Rewrite keyctl_session_to_parent() to run task work on the parent
> > > > > synchronously, so that any errors that happen in the task work can be
> > > > > plumbed back into the syscall return value in the child.
> > > >
> > > > The main thing I worry about is if there's a way to deadlock the child and the
> > > > parent against each other.  vfork() for example.
> > >
> > > Yes - I think it would work fine for scenarios like using
> > > KEYCTL_SESSION_TO_PARENT from a helper binary against the shell that
> > > launched the helper (which I think is the intended usecase?), but
> > > there could theoretically be constellations where it would cause an
> > > (interruptible) hang if the parent is stuck in
> > > uninterruptible/killable sleep.
> > >
> > > I think vfork() is rather special in that it does a killable wait for
> > > the child to exit or execute; and based on my understanding of the
> > > intended usecase of KEYCTL_SESSION_TO_PARENT, I think normally
> > > KEYCTL_SESSION_TO_PARENT would only be used by a child that has gone
> > > through execve?
> >
> > Where did we land on all of this?  Unless I missed a thread somewhere,
> > it looks like the discussion trailed off without any resolution on if
> > we are okay with a potentially (interruptible) deadlock?
>
> As a potential tweak to this, what if we gave up on the idea of
> returning the error code so we could avoid the signal deadlock issue?

I'm still not convinced that there is a real danger of deadlocking
here if the only way to deadlock involves the parent being in an
uninterruptible wait. There aren't many places in the kernel that
involve a parent uninterruptibly waiting for the child without locks
being involved, especially when the parent is a shell that spawns the
child with execve, as seems to be the intended use here.

I really dislike the idea of silently ignoring an error - I kinda feel
like if we give up on returning an error to the child that issued the
keyctl, the next-best option is to SIGKILL the parent, so that we can
say "hey, we technically ensured that all future syscalls in the
parent will use the new creds, because the parent will no longer do
_any_ syscalls".

> I suppose there could be an issue if the parent was
> expecting/depending on keyring change from the child, but honestly, if
> the parent is relying on the kernel keyring and spawning a child
> process without restring the KEYCTL_SESSION_TO_PARENT then the parent
> really should be doing some sanity checks on the keyring after the
> child returns anyway.



> I'm conflicted on the best way to solve this problem, but I think we
> need to fix this somehow as I believe the current behavior is broken
> ...