| Message ID | 1510754620-27088-14-git-send-email-elena.reshetova@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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; }