lkml.org 
[lkml]   [2017]   [Nov]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 13/16] groups: convert group_info.usage to refcount_t
On Wed, Nov 15, 2017 at 6:03 AM, Elena Reshetova
<elena.reshetova@intel.com> wrote:
> 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 group_info.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 lib/refcount.c
> have different memory ordering guarantees than their atomic
> counterparts.
> The full comparison can be seen in
> https://lkml.org/lkml/2017/11/15/57 and it is hopefully soon
> in state to be merged to the documentation tree.
> 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 group_info.usage it might make a difference
> in following places:
> - put_group_info(): decrement in refcount_dec_and_test() only
> provides RELEASE ordering and control dependency on success
> vs. fully ordered atomic counterpart

This looks fine to me: there doesn't appear to be anything special in
the refcounting here.

Acked-by: Kees Cook <keescook@chromium.org>

Andrew, can you pick this up?

Thanks,

-Kees

>
> Suggested-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: David Windsor <dwindsor@gmail.com>
> Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---
> include/linux/cred.h | 7 ++++---
> kernel/cred.c | 2 +-
> kernel/groups.c | 2 +-
> 3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/cred.h b/include/linux/cred.h
> index 099058e..00948dd 100644
> --- a/include/linux/cred.h
> +++ b/include/linux/cred.h
> @@ -17,6 +17,7 @@
> #include <linux/key.h>
> #include <linux/selinux.h>
> #include <linux/atomic.h>
> +#include <linux/refcount.h>
> #include <linux/uidgid.h>
> #include <linux/sched.h>
> #include <linux/sched/user.h>
> @@ -28,7 +29,7 @@ struct inode;
> * COW Supplementary groups list
> */
> struct group_info {
> - atomic_t usage;
> + refcount_t usage;
> int ngroups;
> kgid_t gid[0];
> } __randomize_layout;
> @@ -44,7 +45,7 @@ struct group_info {
> */
> static inline struct group_info *get_group_info(struct group_info *gi)
> {
> - atomic_inc(&gi->usage);
> + refcount_inc(&gi->usage);
> return gi;
> }
>
> @@ -54,7 +55,7 @@ static inline struct group_info *get_group_info(struct group_info *gi)
> */
> #define put_group_info(group_info) \
> do { \
> - if (atomic_dec_and_test(&(group_info)->usage)) \
> + if (refcount_dec_and_test(&(group_info)->usage)) \
> groups_free(group_info); \
> } while (0)
>
> diff --git a/kernel/cred.c b/kernel/cred.c
> index 0192a94..9604c1a 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -36,7 +36,7 @@ do { \
> static struct kmem_cache *cred_jar;
>
> /* init to 2 - one for init_task, one to ensure it is never freed */
> -struct group_info init_groups = { .usage = ATOMIC_INIT(2) };
> +struct group_info init_groups = { .usage = REFCOUNT_INIT(2) };
>
> /*
> * The initial credentials for the initial task
> diff --git a/kernel/groups.c b/kernel/groups.c
> index e357bc8..2ab0e56 100644
> --- a/kernel/groups.c
> +++ b/kernel/groups.c
> @@ -24,7 +24,7 @@ struct group_info *groups_alloc(int gidsetsize)
> if (!gi)
> return NULL;
>
> - atomic_set(&gi->usage, 1);
> + refcount_set(&gi->usage, 1);
> gi->ngroups = gidsetsize;
> return gi;
> }
> --
> 2.7.4
>



--
Kees Cook
Pixel Security
\
 
 \ /
  Last update: 2017-11-17 17:18    [W:0.119 / U:1.196 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site