| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a32e7ee72f47f5e25abd95d4db2fc6d50e32d5a2.camel@kernel.org>
Date: Fri, 18 Aug 2023 16:12:22 -0400
From: Jeff Layton <jlayton@...nel.org>
To: Kees Cook <keescook@...omium.org>, linux-hardening@...r.kernel.org
Cc: Elena Reshetova <elena.reshetova@...el.com>,
David Windsor <dwindsor@...il.com>,
Hans Liljestrand <ishkamiel@...il.com>,
Trond Myklebust <trond.myklebust@...merspace.com>,
Anna Schumaker <anna@...nel.org>,
Chuck Lever <chuck.lever@...cle.com>,
Neil Brown <neilb@...e.de>,
Olga Kornievskaia <kolga@...app.com>,
Dai Ngo <Dai.Ngo@...cle.com>, Tom Talpey <tom@...pey.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Sergey Senozhatsky <senozhatsky@...omium.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Alexey Gladkov <legion@...nel.org>,
"Eric W. Biederman" <ebiederm@...ssion.com>,
Yu Zhao <yuzhao@...gle.com>, linux-kernel@...r.kernel.org,
linux-nfs@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v2] creds: Convert cred.usage to refcount_t
On Thu, 2023-08-17 at 21:17 -0700, Kees Cook wrote:
> From: Elena Reshetova <elena.reshetova@...el.com>
>
> atomic_t variables are currently used to implement reference counters
> with the following properties:
> - counter is initialized to 1 using atomic_set()
> - a resource is freed upon counter reaching zero
> - once counter reaches zero, its further
> increments aren't allowed
> - counter schema uses basic atomic operations
> (set, inc, inc_not_zero, dec_and_test, etc.)
>
> Such atomic variables should be converted to a newly provided
> refcount_t type and API that prevents accidental counter overflows and
> underflows. This is important since overflows and underflows can lead
> to use-after-free situation and be exploitable.
>
> The variable cred.usage is used as pure reference counter. Convert it
> to refcount_t and fix up the operations.
>
> **Important note for maintainers:
>
> Some functions from refcount_t API defined in refcount.h have different
> memory ordering guarantees than their atomic counterparts. Please check
> Documentation/core-api/refcount-vs-atomic.rst for more information.
>
> Normally the differences should not matter since refcount_t provides
> enough guarantees to satisfy the refcounting use cases, but in some
> rare cases it might matter. Please double check that you don't have
> some undocumented memory guarantees for this variable usage.
>
> For the cred.usage it might make a difference in following places:
> - get_task_cred(): increment in refcount_inc_not_zero() only
> guarantees control dependency on success vs. fully ordered atomic
> counterpart
> - put_cred(): decrement in refcount_dec_and_test() only
> provides RELEASE ordering and ACQUIRE ordering on success vs. fully
> ordered atomic counterpart
>
> Suggested-by: Kees Cook <keescook@...omium.org>
> Signed-off-by: Elena Reshetova <elena.reshetova@...el.com>
> Reviewed-by: David Windsor <dwindsor@...il.com>
> Reviewed-by: Hans Liljestrand <ishkamiel@...il.com>
> Signed-off-by: Kees Cook <keescook@...omium.org>
> ---
> v2: rebase
> v1: https://lore.kernel.org/lkml/20200612183450.4189588-4-keescook@chromium.org/
> ---
> include/linux/cred.h | 8 ++++----
> kernel/cred.c | 42 +++++++++++++++++++++---------------------
> net/sunrpc/auth.c | 2 +-
> 3 files changed, 26 insertions(+), 26 deletions(-)
>
> diff --git a/include/linux/cred.h b/include/linux/cred.h
> index 8661f6294ad4..bf1c142afcec 100644
> --- a/include/linux/cred.h
> +++ b/include/linux/cred.h
> @@ -109,7 +109,7 @@ static inline int groups_search(const struct group_info *group_info, kgid_t grp)
> * same context as task->real_cred.
> */
> struct cred {
> - atomic_t usage;
> + refcount_t usage;
> #ifdef CONFIG_DEBUG_CREDENTIALS
> atomic_t subscribers; /* number of processes subscribed */
> void *put_addr;
> @@ -229,7 +229,7 @@ static inline bool cap_ambient_invariant_ok(const struct cred *cred)
> */
> static inline struct cred *get_new_cred(struct cred *cred)
> {
> - atomic_inc(&cred->usage);
> + refcount_inc(&cred->usage);
> return cred;
> }
>
> @@ -261,7 +261,7 @@ static inline const struct cred *get_cred_rcu(const struct cred *cred)
> struct cred *nonconst_cred = (struct cred *) cred;
> if (!cred)
> return NULL;
> - if (!atomic_inc_not_zero(&nonconst_cred->usage))
> + if (!refcount_inc_not_zero(&nonconst_cred->usage))
> return NULL;
> validate_creds(cred);
> nonconst_cred->non_rcu = 0;
> @@ -285,7 +285,7 @@ static inline void put_cred(const struct cred *_cred)
>
> if (cred) {
> validate_creds(cred);
> - if (atomic_dec_and_test(&(cred)->usage))
> + if (refcount_dec_and_test(&(cred)->usage))
> __put_cred(cred);
> }
> }
> diff --git a/kernel/cred.c b/kernel/cred.c
> index bed458cfb812..33090c43bcac 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -39,7 +39,7 @@ static struct group_info init_groups = { .usage = REFCOUNT_INIT(2) };
> * The initial credentials for the initial task
> */
> struct cred init_cred = {
> - .usage = ATOMIC_INIT(4),
> + .usage = REFCOUNT_INIT(4),
> #ifdef CONFIG_DEBUG_CREDENTIALS
> .subscribers = ATOMIC_INIT(2),
> .magic = CRED_MAGIC,
> @@ -99,17 +99,17 @@ static void put_cred_rcu(struct rcu_head *rcu)
>
> #ifdef CONFIG_DEBUG_CREDENTIALS
> if (cred->magic != CRED_MAGIC_DEAD ||
> - atomic_read(&cred->usage) != 0 ||
> + refcount_read(&cred->usage) != 0 ||
> read_cred_subscribers(cred) != 0)
> panic("CRED: put_cred_rcu() sees %p with"
> " mag %x, put %p, usage %d, subscr %d\n",
> cred, cred->magic, cred->put_addr,
> - atomic_read(&cred->usage),
> + refcount_read(&cred->usage),
> read_cred_subscribers(cred));
> #else
> - if (atomic_read(&cred->usage) != 0)
> + if (refcount_read(&cred->usage) != 0)
> panic("CRED: put_cred_rcu() sees %p with usage %d\n",
> - cred, atomic_read(&cred->usage));
> + cred, refcount_read(&cred->usage));
> #endif
>
> security_cred_free(cred);
> @@ -135,10 +135,10 @@ static void put_cred_rcu(struct rcu_head *rcu)
> void __put_cred(struct cred *cred)
> {
> kdebug("__put_cred(%p{%d,%d})", cred,
> - atomic_read(&cred->usage),
> + refcount_read(&cred->usage),
> read_cred_subscribers(cred));
>
> - BUG_ON(atomic_read(&cred->usage) != 0);
> + BUG_ON(refcount_read(&cred->usage) != 0);
> #ifdef CONFIG_DEBUG_CREDENTIALS
> BUG_ON(read_cred_subscribers(cred) != 0);
> cred->magic = CRED_MAGIC_DEAD;
> @@ -162,7 +162,7 @@ void exit_creds(struct task_struct *tsk)
> struct cred *cred;
>
> kdebug("exit_creds(%u,%p,%p,{%d,%d})", tsk->pid, tsk->real_cred, tsk->cred,
> - atomic_read(&tsk->cred->usage),
> + refcount_read(&tsk->cred->usage),
> read_cred_subscribers(tsk->cred));
>
> cred = (struct cred *) tsk->real_cred;
> @@ -221,7 +221,7 @@ struct cred *cred_alloc_blank(void)
> if (!new)
> return NULL;
>
> - atomic_set(&new->usage, 1);
> + refcount_set(&new->usage, 1);
> #ifdef CONFIG_DEBUG_CREDENTIALS
> new->magic = CRED_MAGIC;
> #endif
> @@ -267,7 +267,7 @@ struct cred *prepare_creds(void)
> memcpy(new, old, sizeof(struct cred));
>
> new->non_rcu = 0;
> - atomic_set(&new->usage, 1);
> + refcount_set(&new->usage, 1);
> set_cred_subscribers(new, 0);
> get_group_info(new->group_info);
> get_uid(new->user);
> @@ -356,7 +356,7 @@ int copy_creds(struct task_struct *p, unsigned long clone_flags)
> get_cred(p->cred);
> alter_cred_subscribers(p->cred, 2);
> kdebug("share_creds(%p{%d,%d})",
> - p->cred, atomic_read(&p->cred->usage),
> + p->cred, refcount_read(&p->cred->usage),
> read_cred_subscribers(p->cred));
> inc_rlimit_ucounts(task_ucounts(p), UCOUNT_RLIMIT_NPROC, 1);
> return 0;
> @@ -450,7 +450,7 @@ int commit_creds(struct cred *new)
> const struct cred *old = task->real_cred;
>
> kdebug("commit_creds(%p{%d,%d})", new,
> - atomic_read(&new->usage),
> + refcount_read(&new->usage),
> read_cred_subscribers(new));
>
> BUG_ON(task->cred != old);
> @@ -459,7 +459,7 @@ int commit_creds(struct cred *new)
> validate_creds(old);
> validate_creds(new);
> #endif
> - BUG_ON(atomic_read(&new->usage) < 1);
> + BUG_ON(refcount_read(&new->usage) < 1);
>
> get_cred(new); /* we will require a ref for the subj creds too */
>
> @@ -533,13 +533,13 @@ EXPORT_SYMBOL(commit_creds);
> void abort_creds(struct cred *new)
> {
> kdebug("abort_creds(%p{%d,%d})", new,
> - atomic_read(&new->usage),
> + refcount_read(&new->usage),
> read_cred_subscribers(new));
>
> #ifdef CONFIG_DEBUG_CREDENTIALS
> BUG_ON(read_cred_subscribers(new) != 0);
> #endif
> - BUG_ON(atomic_read(&new->usage) < 1);
> + BUG_ON(refcount_read(&new->usage) < 1);
> put_cred(new);
> }
> EXPORT_SYMBOL(abort_creds);
> @@ -556,7 +556,7 @@ const struct cred *override_creds(const struct cred *new)
> const struct cred *old = current->cred;
>
> kdebug("override_creds(%p{%d,%d})", new,
> - atomic_read(&new->usage),
> + refcount_read(&new->usage),
> read_cred_subscribers(new));
>
> validate_creds(old);
> @@ -579,7 +579,7 @@ const struct cred *override_creds(const struct cred *new)
> alter_cred_subscribers(old, -1);
>
> kdebug("override_creds() = %p{%d,%d}", old,
> - atomic_read(&old->usage),
> + refcount_read(&old->usage),
> read_cred_subscribers(old));
> return old;
> }
> @@ -597,7 +597,7 @@ void revert_creds(const struct cred *old)
> const struct cred *override = current->cred;
>
> kdebug("revert_creds(%p{%d,%d})", old,
> - atomic_read(&old->usage),
> + refcount_read(&old->usage),
> read_cred_subscribers(old));
>
> validate_creds(old);
> @@ -728,7 +728,7 @@ struct cred *prepare_kernel_cred(struct task_struct *daemon)
>
> *new = *old;
> new->non_rcu = 0;
> - atomic_set(&new->usage, 1);
> + refcount_set(&new->usage, 1);
> set_cred_subscribers(new, 0);
> get_uid(new->user);
> get_user_ns(new->user_ns);
> @@ -843,7 +843,7 @@ static void dump_invalid_creds(const struct cred *cred, const char *label,
> printk(KERN_ERR "CRED: ->magic=%x, put_addr=%p\n",
> cred->magic, cred->put_addr);
> printk(KERN_ERR "CRED: ->usage=%d, subscr=%d\n",
> - atomic_read(&cred->usage),
> + refcount_read(&cred->usage),
> read_cred_subscribers(cred));
> printk(KERN_ERR "CRED: ->*uid = { %d,%d,%d,%d }\n",
> from_kuid_munged(&init_user_ns, cred->uid),
> @@ -917,7 +917,7 @@ void validate_creds_for_do_exit(struct task_struct *tsk)
> {
> kdebug("validate_creds_for_do_exit(%p,%p{%d,%d})",
> tsk->real_cred, tsk->cred,
> - atomic_read(&tsk->cred->usage),
> + refcount_read(&tsk->cred->usage),
> read_cred_subscribers(tsk->cred));
>
> __validate_process_creds(tsk, __FILE__, __LINE__);
> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> index 2f16f9d17966..f9f406249e7d 100644
> --- a/net/sunrpc/auth.c
> +++ b/net/sunrpc/auth.c
> @@ -39,7 +39,7 @@ static LIST_HEAD(cred_unused);
> static unsigned long number_cred_unused;
>
> static struct cred machine_cred = {
> - .usage = ATOMIC_INIT(1),
> + .usage = REFCOUNT_INIT(1),
> #ifdef CONFIG_DEBUG_CREDENTIALS
> .magic = CRED_MAGIC,
> #endif
I wonder what sort of bugs this will uncover.
Reviewed-by: Jeff Layton <jlayton@...nel.org>
Powered by blists - more mailing lists