* [PATCH] selinux: convert struct sidtab count to refcount_t @ 2019-07-22 11:31 NitinGote 2019-07-22 13:17 ` Ondrej Mosnacek 0 siblings, 1 reply; 11+ messages in thread From: NitinGote @ 2019-07-22 11:31 UTC (permalink / raw) To: keescook Cc: kernel-hardening, paul, sds, eparis, omosnace, selinux, linux-kernel, NitinGote refcount_t type and corresponding API should be used instead of atomic_t when the variable is used as a reference counter. This allows to avoid accidental refcounter overflows that might lead to use-after-free situations. Signed-off-by: NitinGote <nitin.r.gote@intel.com> --- security/selinux/ss/sidtab.c | 16 ++++++++-------- security/selinux/ss/sidtab.h | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c index e63a90ff2728..20fe235c6c71 100644 --- a/security/selinux/ss/sidtab.c +++ b/security/selinux/ss/sidtab.c @@ -29,7 +29,7 @@ int sidtab_init(struct sidtab *s) for (i = 0; i < SECINITSID_NUM; i++) s->isids[i].set = 0; - atomic_set(&s->count, 0); + refcount_set(&s->count, 0); s->convert = NULL; @@ -130,7 +130,7 @@ static struct context *sidtab_do_lookup(struct sidtab *s, u32 index, int alloc) static struct context *sidtab_lookup(struct sidtab *s, u32 index) { - u32 count = (u32)atomic_read(&s->count); + u32 count = refcount_read(&s->count); if (index >= count) return NULL; @@ -245,7 +245,7 @@ static int sidtab_reverse_lookup(struct sidtab *s, struct context *context, u32 *index) { unsigned long flags; - u32 count = (u32)atomic_read(&s->count); + u32 count = (u32)refcount_read(&s->count); u32 count_locked, level, pos; struct sidtab_convert_params *convert; struct context *dst, *dst_convert; @@ -272,7 +272,7 @@ static int sidtab_reverse_lookup(struct sidtab *s, struct context *context, spin_lock_irqsave(&s->lock, flags); convert = s->convert; - count_locked = (u32)atomic_read(&s->count); + count_locked = (u32)refcount_read(&s->count); level = sidtab_level_from_count(count_locked); /* if count has changed before we acquired the lock, then catch up */ @@ -315,7 +315,7 @@ static int sidtab_reverse_lookup(struct sidtab *s, struct context *context, } /* at this point we know the insert won't fail */ - atomic_set(&convert->target->count, count + 1); + refcount_set(&convert->target->count, count + 1); } if (context->len) @@ -328,7 +328,7 @@ static int sidtab_reverse_lookup(struct sidtab *s, struct context *context, /* write entries before writing new count */ smp_wmb(); - atomic_set(&s->count, count + 1); + refcount_set(&s->count, count + 1); rc = 0; out_unlock: @@ -418,7 +418,7 @@ int sidtab_convert(struct sidtab *s, struct sidtab_convert_params *params) return -EBUSY; } - count = (u32)atomic_read(&s->count); + count = (u32)refcount_read(&s->count); level = sidtab_level_from_count(count); /* allocate last leaf in the new sidtab (to avoid race with @@ -431,7 +431,7 @@ int sidtab_convert(struct sidtab *s, struct sidtab_convert_params *params) } /* set count in case no new entries are added during conversion */ - atomic_set(¶ms->target->count, count); + refcount_set(¶ms->target->count, count); /* enable live convert of new entries */ s->convert = params; diff --git a/security/selinux/ss/sidtab.h b/security/selinux/ss/sidtab.h index bbd5c0d1f3bd..68dd96a5beba 100644 --- a/security/selinux/ss/sidtab.h +++ b/security/selinux/ss/sidtab.h @@ -70,7 +70,7 @@ struct sidtab_convert_params { struct sidtab { union sidtab_entry_inner roots[SIDTAB_MAX_LEVEL + 1]; - atomic_t count; + refcount_t count; struct sidtab_convert_params *convert; spinlock_t lock; -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] selinux: convert struct sidtab count to refcount_t 2019-07-22 11:31 [PATCH] selinux: convert struct sidtab count to refcount_t NitinGote @ 2019-07-22 13:17 ` Ondrej Mosnacek 2019-07-23 0:25 ` Paul Moore ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Ondrej Mosnacek @ 2019-07-22 13:17 UTC (permalink / raw) To: NitinGote Cc: Kees Cook, kernel-hardening, Paul Moore, Stephen Smalley, Eric Paris, SElinux list, Linux kernel mailing list On Mon, Jul 22, 2019 at 1:35 PM NitinGote <nitin.r.gote@intel.com> wrote: > refcount_t type and corresponding API should be > used instead of atomic_t when the variable is used as > a reference counter. This allows to avoid accidental > refcounter overflows that might lead to use-after-free > situations. > > Signed-off-by: NitinGote <nitin.r.gote@intel.com> Nack. The 'count' variable is not used as a reference counter here. It tracks the number of entries in sidtab, which is a very specific lookup table that can only grow (the count never decreases). I only made it atomic because the variable is read outside of the sidtab's spin lock and thus the reads and writes to it need to be guaranteed to be atomic. The counter is only updated under the spin lock, so insertions do not race with each other. Your patch, however, lead me to realize that I forgot to guard against overflow above SIDTAB_MAX when a new entry is being inserted. It is extremely unlikely to happen in practice, but should be fixed anyway. I'll send a patch shortly. > --- > security/selinux/ss/sidtab.c | 16 ++++++++-------- > security/selinux/ss/sidtab.h | 2 +- > 2 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c > index e63a90ff2728..20fe235c6c71 100644 > --- a/security/selinux/ss/sidtab.c > +++ b/security/selinux/ss/sidtab.c > @@ -29,7 +29,7 @@ int sidtab_init(struct sidtab *s) > for (i = 0; i < SECINITSID_NUM; i++) > s->isids[i].set = 0; > > - atomic_set(&s->count, 0); > + refcount_set(&s->count, 0); > > s->convert = NULL; > > @@ -130,7 +130,7 @@ static struct context *sidtab_do_lookup(struct sidtab *s, u32 index, int alloc) > > static struct context *sidtab_lookup(struct sidtab *s, u32 index) > { > - u32 count = (u32)atomic_read(&s->count); > + u32 count = refcount_read(&s->count); > > if (index >= count) > return NULL; > @@ -245,7 +245,7 @@ static int sidtab_reverse_lookup(struct sidtab *s, struct context *context, > u32 *index) > { > unsigned long flags; > - u32 count = (u32)atomic_read(&s->count); > + u32 count = (u32)refcount_read(&s->count); > u32 count_locked, level, pos; > struct sidtab_convert_params *convert; > struct context *dst, *dst_convert; > @@ -272,7 +272,7 @@ static int sidtab_reverse_lookup(struct sidtab *s, struct context *context, > spin_lock_irqsave(&s->lock, flags); > > convert = s->convert; > - count_locked = (u32)atomic_read(&s->count); > + count_locked = (u32)refcount_read(&s->count); > level = sidtab_level_from_count(count_locked); > > /* if count has changed before we acquired the lock, then catch up */ > @@ -315,7 +315,7 @@ static int sidtab_reverse_lookup(struct sidtab *s, struct context *context, > } > > /* at this point we know the insert won't fail */ > - atomic_set(&convert->target->count, count + 1); > + refcount_set(&convert->target->count, count + 1); > } > > if (context->len) > @@ -328,7 +328,7 @@ static int sidtab_reverse_lookup(struct sidtab *s, struct context *context, > /* write entries before writing new count */ > smp_wmb(); > > - atomic_set(&s->count, count + 1); > + refcount_set(&s->count, count + 1); > > rc = 0; > out_unlock: > @@ -418,7 +418,7 @@ int sidtab_convert(struct sidtab *s, struct sidtab_convert_params *params) > return -EBUSY; > } > > - count = (u32)atomic_read(&s->count); > + count = (u32)refcount_read(&s->count); > level = sidtab_level_from_count(count); > > /* allocate last leaf in the new sidtab (to avoid race with > @@ -431,7 +431,7 @@ int sidtab_convert(struct sidtab *s, struct sidtab_convert_params *params) > } > > /* set count in case no new entries are added during conversion */ > - atomic_set(¶ms->target->count, count); > + refcount_set(¶ms->target->count, count); > > /* enable live convert of new entries */ > s->convert = params; > diff --git a/security/selinux/ss/sidtab.h b/security/selinux/ss/sidtab.h > index bbd5c0d1f3bd..68dd96a5beba 100644 > --- a/security/selinux/ss/sidtab.h > +++ b/security/selinux/ss/sidtab.h > @@ -70,7 +70,7 @@ struct sidtab_convert_params { > > struct sidtab { > union sidtab_entry_inner roots[SIDTAB_MAX_LEVEL + 1]; > - atomic_t count; > + refcount_t count; > struct sidtab_convert_params *convert; > spinlock_t lock; > > -- > 2.17.1 > Thanks, -- Ondrej Mosnacek <omosnace at redhat dot com> Software Engineer, Security Technologies Red Hat, Inc. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] selinux: convert struct sidtab count to refcount_t 2019-07-22 13:17 ` Ondrej Mosnacek @ 2019-07-23 0:25 ` Paul Moore 2019-07-23 5:44 ` Gote, Nitin R 2019-07-23 14:53 ` Jann Horn 2 siblings, 0 replies; 11+ messages in thread From: Paul Moore @ 2019-07-23 0:25 UTC (permalink / raw) To: Ondrej Mosnacek, NitinGote Cc: Kees Cook, kernel-hardening, Stephen Smalley, Eric Paris, SElinux list, Linux kernel mailing list On Mon, Jul 22, 2019 at 9:18 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Mon, Jul 22, 2019 at 1:35 PM NitinGote <nitin.r.gote@intel.com> wrote: > > refcount_t type and corresponding API should be > > used instead of atomic_t when the variable is used as > > a reference counter. This allows to avoid accidental > > refcounter overflows that might lead to use-after-free > > situations. > > > > Signed-off-by: NitinGote <nitin.r.gote@intel.com> > > Nack. > > The 'count' variable is not used as a reference counter here. It > tracks the number of entries in sidtab, which is a very specific > lookup table that can only grow (the count never decreases). I only > made it atomic because the variable is read outside of the sidtab's > spin lock and thus the reads and writes to it need to be guaranteed to > be atomic. The counter is only updated under the spin lock, so > insertions do not race with each other. Agreed, this should be changed to use refcount_t. -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] selinux: convert struct sidtab count to refcount_t 2019-07-22 13:17 ` Ondrej Mosnacek 2019-07-23 0:25 ` Paul Moore @ 2019-07-23 5:44 ` Gote, Nitin R 2019-07-23 14:53 ` Jann Horn 2 siblings, 0 replies; 11+ messages in thread From: Gote, Nitin R @ 2019-07-23 5:44 UTC (permalink / raw) To: Ondrej Mosnacek Cc: Kees Cook, kernel-hardening, Paul Moore, Stephen Smalley, Eric Paris, SElinux list, Linux kernel mailing list > -----Original Message----- > From: Ondrej Mosnacek [mailto:omosnace@redhat.com] > Sent: Monday, July 22, 2019 6:48 PM > To: Gote, Nitin R <nitin.r.gote@intel.com> > Cc: Kees Cook <keescook@chromium.org>; kernel- > hardening@lists.openwall.com; Paul Moore <paul@paul-moore.com>; > Stephen Smalley <sds@tycho.nsa.gov>; Eric Paris <eparis@parisplace.org>; > SElinux list <selinux@vger.kernel.org>; Linux kernel mailing list <linux- > kernel@vger.kernel.org> > Subject: Re: [PATCH] selinux: convert struct sidtab count to refcount_t > > On Mon, Jul 22, 2019 at 1:35 PM NitinGote <nitin.r.gote@intel.com> wrote: > > refcount_t type and corresponding API should be used instead of > > atomic_t when the variable is used as a reference counter. This allows > > to avoid accidental refcounter overflows that might lead to > > use-after-free situations. > > > > Signed-off-by: NitinGote <nitin.r.gote@intel.com> > > Nack. > > The 'count' variable is not used as a reference counter here. It tracks the > number of entries in sidtab, which is a very specific lookup table that can > only grow (the count never decreases). I only made it atomic because the > variable is read outside of the sidtab's spin lock and thus the reads and > writes to it need to be guaranteed to be atomic. The counter is only updated > under the spin lock, so insertions do not race with each other. Agreed. Thanks for clarification. I'm going to discontinue this patch. > > Your patch, however, lead me to realize that I forgot to guard against > overflow above SIDTAB_MAX when a new entry is being inserted. It is > extremely unlikely to happen in practice, but should be fixed anyway. > I'll send a patch shortly. > Thank you. > > --- > > security/selinux/ss/sidtab.c | 16 ++++++++-------- > > security/selinux/ss/sidtab.h | 2 +- > > 2 files changed, 9 insertions(+), 9 deletions(-) > > > > diff --git a/security/selinux/ss/sidtab.c > > b/security/selinux/ss/sidtab.c index e63a90ff2728..20fe235c6c71 100644 > > --- a/security/selinux/ss/sidtab.c > > +++ b/security/selinux/ss/sidtab.c > > @@ -29,7 +29,7 @@ int sidtab_init(struct sidtab *s) > > for (i = 0; i < SECINITSID_NUM; i++) > > s->isids[i].set = 0; > > > > - atomic_set(&s->count, 0); > > + refcount_set(&s->count, 0); > > > > s->convert = NULL; > > > > @@ -130,7 +130,7 @@ static struct context *sidtab_do_lookup(struct > > sidtab *s, u32 index, int alloc) > > > > static struct context *sidtab_lookup(struct sidtab *s, u32 index) { > > - u32 count = (u32)atomic_read(&s->count); > > + u32 count = refcount_read(&s->count); > > > > if (index >= count) > > return NULL; > > @@ -245,7 +245,7 @@ static int sidtab_reverse_lookup(struct sidtab *s, > struct context *context, > > u32 *index) > > { > > unsigned long flags; > > - u32 count = (u32)atomic_read(&s->count); > > + u32 count = (u32)refcount_read(&s->count); > > u32 count_locked, level, pos; > > struct sidtab_convert_params *convert; > > struct context *dst, *dst_convert; > > @@ -272,7 +272,7 @@ static int sidtab_reverse_lookup(struct sidtab *s, > struct context *context, > > spin_lock_irqsave(&s->lock, flags); > > > > convert = s->convert; > > - count_locked = (u32)atomic_read(&s->count); > > + count_locked = (u32)refcount_read(&s->count); > > level = sidtab_level_from_count(count_locked); > > > > /* if count has changed before we acquired the lock, then catch up */ > > @@ -315,7 +315,7 @@ static int sidtab_reverse_lookup(struct sidtab *s, > struct context *context, > > } > > > > /* at this point we know the insert won't fail */ > > - atomic_set(&convert->target->count, count + 1); > > + refcount_set(&convert->target->count, count + 1); > > } > > > > if (context->len) > > @@ -328,7 +328,7 @@ static int sidtab_reverse_lookup(struct sidtab *s, > struct context *context, > > /* write entries before writing new count */ > > smp_wmb(); > > > > - atomic_set(&s->count, count + 1); > > + refcount_set(&s->count, count + 1); > > > > rc = 0; > > out_unlock: > > @@ -418,7 +418,7 @@ int sidtab_convert(struct sidtab *s, struct > sidtab_convert_params *params) > > return -EBUSY; > > } > > > > - count = (u32)atomic_read(&s->count); > > + count = (u32)refcount_read(&s->count); > > level = sidtab_level_from_count(count); > > > > /* allocate last leaf in the new sidtab (to avoid race with > > @@ -431,7 +431,7 @@ int sidtab_convert(struct sidtab *s, struct > sidtab_convert_params *params) > > } > > > > /* set count in case no new entries are added during conversion */ > > - atomic_set(¶ms->target->count, count); > > + refcount_set(¶ms->target->count, count); > > > > /* enable live convert of new entries */ > > s->convert = params; > > diff --git a/security/selinux/ss/sidtab.h b/security/selinux/ss/sidtab.h > > index bbd5c0d1f3bd..68dd96a5beba 100644 > > --- a/security/selinux/ss/sidtab.h > > +++ b/security/selinux/ss/sidtab.h > > @@ -70,7 +70,7 @@ struct sidtab_convert_params { > > > > struct sidtab { > > union sidtab_entry_inner roots[SIDTAB_MAX_LEVEL + 1]; > > - atomic_t count; > > + refcount_t count; > > struct sidtab_convert_params *convert; > > spinlock_t lock; > > > > -- > > 2.17.1 > > > > Thanks, > > -- > Ondrej Mosnacek <omosnace at redhat dot com> > Software Engineer, Security Technologies > Red Hat, Inc. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] selinux: convert struct sidtab count to refcount_t 2019-07-22 13:17 ` Ondrej Mosnacek 2019-07-23 0:25 ` Paul Moore 2019-07-23 5:44 ` Gote, Nitin R @ 2019-07-23 14:53 ` Jann Horn 2019-07-23 22:17 ` Kees Cook 2019-07-24 16:17 ` Ondrej Mosnacek 2 siblings, 2 replies; 11+ messages in thread From: Jann Horn @ 2019-07-23 14:53 UTC (permalink / raw) To: Ondrej Mosnacek Cc: NitinGote, Kees Cook, Kernel Hardening, Paul Moore, Stephen Smalley, Eric Paris, SElinux list, Linux kernel mailing list On Mon, Jul 22, 2019 at 3:44 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Mon, Jul 22, 2019 at 1:35 PM NitinGote <nitin.r.gote@intel.com> wrote: > > refcount_t type and corresponding API should be > > used instead of atomic_t when the variable is used as > > a reference counter. This allows to avoid accidental > > refcounter overflows that might lead to use-after-free > > situations. > > > > Signed-off-by: NitinGote <nitin.r.gote@intel.com> > > Nack. > > The 'count' variable is not used as a reference counter here. It > tracks the number of entries in sidtab, which is a very specific > lookup table that can only grow (the count never decreases). I only > made it atomic because the variable is read outside of the sidtab's > spin lock and thus the reads and writes to it need to be guaranteed to > be atomic. The counter is only updated under the spin lock, so > insertions do not race with each other. Probably shouldn't even be atomic_t... quoting Documentation/atomic_t.txt: | SEMANTICS | --------- | | Non-RMW ops: | | The non-RMW ops are (typically) regular LOADs and STOREs and are canonically | implemented using READ_ONCE(), WRITE_ONCE(), smp_load_acquire() and | smp_store_release() respectively. Therefore, if you find yourself only using | the Non-RMW operations of atomic_t, you do not in fact need atomic_t at all | and are doing it wrong. So I think what you actually want here is a plain "int count", and then: - for unlocked reads, either READ_ONCE()+smp_rmb() or smp_load_acquire() - for writes, either smp_wmb()+WRITE_ONCE() or smp_store_release() smp_load_acquire() and smp_store_release() are probably the nicest here, since they are semantically clearer than smp_rmb()/smp_wmb(). ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] selinux: convert struct sidtab count to refcount_t 2019-07-23 14:53 ` Jann Horn @ 2019-07-23 22:17 ` Kees Cook 2019-07-24 14:28 ` Jann Horn 2019-07-24 16:17 ` Ondrej Mosnacek 1 sibling, 1 reply; 11+ messages in thread From: Kees Cook @ 2019-07-23 22:17 UTC (permalink / raw) To: Jann Horn Cc: Ondrej Mosnacek, NitinGote, Kernel Hardening, Paul Moore, Stephen Smalley, Eric Paris, SElinux list, Linux kernel mailing list On Tue, Jul 23, 2019 at 04:53:47PM +0200, Jann Horn wrote: > On Mon, Jul 22, 2019 at 3:44 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Mon, Jul 22, 2019 at 1:35 PM NitinGote <nitin.r.gote@intel.com> wrote: > > > refcount_t type and corresponding API should be > > > used instead of atomic_t when the variable is used as > > > a reference counter. This allows to avoid accidental > > > refcounter overflows that might lead to use-after-free > > > situations. > > > > > > Signed-off-by: NitinGote <nitin.r.gote@intel.com> > > > > Nack. > > > > The 'count' variable is not used as a reference counter here. It > > tracks the number of entries in sidtab, which is a very specific > > lookup table that can only grow (the count never decreases). I only > > made it atomic because the variable is read outside of the sidtab's > > spin lock and thus the reads and writes to it need to be guaranteed to > > be atomic. The counter is only updated under the spin lock, so > > insertions do not race with each other. > > Probably shouldn't even be atomic_t... quoting Documentation/atomic_t.txt: > > | SEMANTICS > | --------- > | > | Non-RMW ops: > | > | The non-RMW ops are (typically) regular LOADs and STOREs and are canonically > | implemented using READ_ONCE(), WRITE_ONCE(), smp_load_acquire() and > | smp_store_release() respectively. Therefore, if you find yourself only using > | the Non-RMW operations of atomic_t, you do not in fact need atomic_t at all > | and are doing it wrong. > > So I think what you actually want here is a plain "int count", and then: > - for unlocked reads, either READ_ONCE()+smp_rmb() or smp_load_acquire() > - for writes, either smp_wmb()+WRITE_ONCE() or smp_store_release() > > smp_load_acquire() and smp_store_release() are probably the nicest > here, since they are semantically clearer than smp_rmb()/smp_wmb(). Perhaps we need a "statistics" counter type for these kinds of counters? "counter_t"? I bet there are a lot of atomic_t uses that are just trying to be counters. (likely most of atomic_t that isn't now refcount_t ...) -- Kees Cook ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] selinux: convert struct sidtab count to refcount_t 2019-07-23 22:17 ` Kees Cook @ 2019-07-24 14:28 ` Jann Horn 2019-07-24 15:54 ` Kees Cook 0 siblings, 1 reply; 11+ messages in thread From: Jann Horn @ 2019-07-24 14:28 UTC (permalink / raw) To: Kees Cook Cc: Ondrej Mosnacek, NitinGote, Kernel Hardening, Paul Moore, Stephen Smalley, Eric Paris, SElinux list, Linux kernel mailing list On Wed, Jul 24, 2019 at 12:17 AM Kees Cook <keescook@chromium.org> wrote: > On Tue, Jul 23, 2019 at 04:53:47PM +0200, Jann Horn wrote: > > On Mon, Jul 22, 2019 at 3:44 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > On Mon, Jul 22, 2019 at 1:35 PM NitinGote <nitin.r.gote@intel.com> wrote: > > > > refcount_t type and corresponding API should be > > > > used instead of atomic_t when the variable is used as > > > > a reference counter. This allows to avoid accidental > > > > refcounter overflows that might lead to use-after-free > > > > situations. > > > > > > > > Signed-off-by: NitinGote <nitin.r.gote@intel.com> > > > > > > Nack. > > > > > > The 'count' variable is not used as a reference counter here. It > > > tracks the number of entries in sidtab, which is a very specific > > > lookup table that can only grow (the count never decreases). I only > > > made it atomic because the variable is read outside of the sidtab's > > > spin lock and thus the reads and writes to it need to be guaranteed to > > > be atomic. The counter is only updated under the spin lock, so > > > insertions do not race with each other. > > > > Probably shouldn't even be atomic_t... quoting Documentation/atomic_t.txt: > > > > | SEMANTICS > > | --------- > > | > > | Non-RMW ops: > > | > > | The non-RMW ops are (typically) regular LOADs and STOREs and are canonically > > | implemented using READ_ONCE(), WRITE_ONCE(), smp_load_acquire() and > > | smp_store_release() respectively. Therefore, if you find yourself only using > > | the Non-RMW operations of atomic_t, you do not in fact need atomic_t at all > > | and are doing it wrong. > > > > So I think what you actually want here is a plain "int count", and then: > > - for unlocked reads, either READ_ONCE()+smp_rmb() or smp_load_acquire() > > - for writes, either smp_wmb()+WRITE_ONCE() or smp_store_release() > > > > smp_load_acquire() and smp_store_release() are probably the nicest > > here, since they are semantically clearer than smp_rmb()/smp_wmb(). > > Perhaps we need a "statistics" counter type for these kinds of counters? > "counter_t"? I bet there are a lot of atomic_t uses that are just trying > to be counters. (likely most of atomic_t that isn't now refcount_t ...) This isn't a statistics counter though; this thing needs ordered memory accesses, which you wouldn't need for statistics. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] selinux: convert struct sidtab count to refcount_t 2019-07-24 14:28 ` Jann Horn @ 2019-07-24 15:54 ` Kees Cook 2019-07-24 16:55 ` Jann Horn 0 siblings, 1 reply; 11+ messages in thread From: Kees Cook @ 2019-07-24 15:54 UTC (permalink / raw) To: Jann Horn Cc: Ondrej Mosnacek, NitinGote, Kernel Hardening, Paul Moore, Stephen Smalley, Eric Paris, SElinux list, Linux kernel mailing list On Wed, Jul 24, 2019 at 04:28:31PM +0200, Jann Horn wrote: > On Wed, Jul 24, 2019 at 12:17 AM Kees Cook <keescook@chromium.org> wrote: > > Perhaps we need a "statistics" counter type for these kinds of counters? > > "counter_t"? I bet there are a lot of atomic_t uses that are just trying > > to be counters. (likely most of atomic_t that isn't now refcount_t ...) > > This isn't a statistics counter though; this thing needs ordered > memory accesses, which you wouldn't need for statistics. Okay, it'd be a "very accurate" counter type? It _could_ be used for statistics. I guess what I mean is that there are a lot of places using atomic_t just for upward counting that don't care about wrapping, etc. -- Kees Cook ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] selinux: convert struct sidtab count to refcount_t 2019-07-24 15:54 ` Kees Cook @ 2019-07-24 16:55 ` Jann Horn 2019-07-29 16:51 ` Kees Cook 0 siblings, 1 reply; 11+ messages in thread From: Jann Horn @ 2019-07-24 16:55 UTC (permalink / raw) To: Kees Cook Cc: Ondrej Mosnacek, NitinGote, Kernel Hardening, Paul Moore, Stephen Smalley, Eric Paris, SElinux list, Linux kernel mailing list On Wed, Jul 24, 2019 at 5:54 PM Kees Cook <keescook@chromium.org> wrote: > On Wed, Jul 24, 2019 at 04:28:31PM +0200, Jann Horn wrote: > > On Wed, Jul 24, 2019 at 12:17 AM Kees Cook <keescook@chromium.org> wrote: > > > Perhaps we need a "statistics" counter type for these kinds of counters? > > > "counter_t"? I bet there are a lot of atomic_t uses that are just trying > > > to be counters. (likely most of atomic_t that isn't now refcount_t ...) > > > > This isn't a statistics counter though; this thing needs ordered > > memory accesses, which you wouldn't need for statistics. > > Okay, it'd be a "very accurate" counter type? It _could_ be used for > statistics. I guess what I mean is that there are a lot of places using > atomic_t just for upward counting that don't care about wrapping, etc. (Accurate) statistics counters need RMW ops, don't need memory ordering, usually can't be locked against writes, and may not care about wrapping. This thing doesn't need RMW ops, does need memory ordering, can be locked against writes, and definitely shouldn't wrap. I agree that there are a bunch of statistics counters in the kernel, and it's not necessarily a bad idea to use a separate type for them; but this is not a statistics counter. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] selinux: convert struct sidtab count to refcount_t 2019-07-24 16:55 ` Jann Horn @ 2019-07-29 16:51 ` Kees Cook 0 siblings, 0 replies; 11+ messages in thread From: Kees Cook @ 2019-07-29 16:51 UTC (permalink / raw) To: Jann Horn Cc: Ondrej Mosnacek, NitinGote, Kernel Hardening, Paul Moore, Stephen Smalley, Eric Paris, SElinux list, Linux kernel mailing list On Wed, Jul 24, 2019 at 06:55:47PM +0200, Jann Horn wrote: > (Accurate) statistics counters need RMW ops, don't need memory > ordering, usually can't be locked against writes, and may not care > about wrapping. > This thing doesn't need RMW ops, does need memory ordering, can be > locked against writes, and definitely shouldn't wrap. > > I agree that there are a bunch of statistics counters in the kernel, > and it's not necessarily a bad idea to use a separate type for them; > but this is not a statistics counter. Right, yes, I didn't meant to suggest it should be. I was just bringing up the "counter type" idea again, since it was on my mind here originally. -- Kees Cook ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] selinux: convert struct sidtab count to refcount_t 2019-07-23 14:53 ` Jann Horn 2019-07-23 22:17 ` Kees Cook @ 2019-07-24 16:17 ` Ondrej Mosnacek 1 sibling, 0 replies; 11+ messages in thread From: Ondrej Mosnacek @ 2019-07-24 16:17 UTC (permalink / raw) To: Jann Horn Cc: NitinGote, Kees Cook, Kernel Hardening, Paul Moore, Stephen Smalley, Eric Paris, SElinux list, Linux kernel mailing list On Tue, Jul 23, 2019 at 4:54 PM Jann Horn <jannh@google.com> wrote: > On Mon, Jul 22, 2019 at 3:44 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Mon, Jul 22, 2019 at 1:35 PM NitinGote <nitin.r.gote@intel.com> wrote: > > > refcount_t type and corresponding API should be > > > used instead of atomic_t when the variable is used as > > > a reference counter. This allows to avoid accidental > > > refcounter overflows that might lead to use-after-free > > > situations. > > > > > > Signed-off-by: NitinGote <nitin.r.gote@intel.com> > > > > Nack. > > > > The 'count' variable is not used as a reference counter here. It > > tracks the number of entries in sidtab, which is a very specific > > lookup table that can only grow (the count never decreases). I only > > made it atomic because the variable is read outside of the sidtab's > > spin lock and thus the reads and writes to it need to be guaranteed to > > be atomic. The counter is only updated under the spin lock, so > > insertions do not race with each other. > > Probably shouldn't even be atomic_t... quoting Documentation/atomic_t.txt: > > | SEMANTICS > | --------- > | > | Non-RMW ops: > | > | The non-RMW ops are (typically) regular LOADs and STOREs and are canonically > | implemented using READ_ONCE(), WRITE_ONCE(), smp_load_acquire() and > | smp_store_release() respectively. Therefore, if you find yourself only using > | the Non-RMW operations of atomic_t, you do not in fact need atomic_t at all > | and are doing it wrong. > > So I think what you actually want here is a plain "int count", and then: > - for unlocked reads, either READ_ONCE()+smp_rmb() or smp_load_acquire() > - for writes, either smp_wmb()+WRITE_ONCE() or smp_store_release() > > smp_load_acquire() and smp_store_release() are probably the nicest > here, since they are semantically clearer than smp_rmb()/smp_wmb(). Oh yes, I had a hunch that there would be a better way to do it... I should have taken the time to read the documentation carefully :) I am on PTO today, but I will be happy to send a patch to convert the atomic_t usage to the smp_load_acquire()/smp_store_release() helpers tomorrow. It will also allow us to just use u32 directly and to get rid of the ugly casts and the INT_MAX limit. Thanks a lot for the hint, Jann! -- Ondrej Mosnacek <omosnace at redhat dot com> Software Engineer, Security Technologies Red Hat, Inc. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-07-29 16:51 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-22 11:31 [PATCH] selinux: convert struct sidtab count to refcount_t NitinGote 2019-07-22 13:17 ` Ondrej Mosnacek 2019-07-23 0:25 ` Paul Moore 2019-07-23 5:44 ` Gote, Nitin R 2019-07-23 14:53 ` Jann Horn 2019-07-23 22:17 ` Kees Cook 2019-07-24 14:28 ` Jann Horn 2019-07-24 15:54 ` Kees Cook 2019-07-24 16:55 ` Jann Horn 2019-07-29 16:51 ` Kees Cook 2019-07-24 16:17 ` Ondrej Mosnacek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).