[PATCH] selinux: convert struct sidtab count to refcount_t

[PATCH] selinux: convert struct sidtab count to refcount_t

[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(&params->target->count, count);
+	refcount_set(&params->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

Re: [PATCH] selinux: convert struct sidtab count to refcount_t

[Ondrej Mosnacek]

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(&params->target->count, count);
> +       refcount_set(&params->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.

Re: [PATCH] selinux: convert struct sidtab count to refcount_t

[Paul Moore]

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

RE: [PATCH] selinux: convert struct sidtab count to refcount_t

[Gote, Nitin R]

> -----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(&params->target->count, count);
> > +       refcount_set(&params->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.

Re: [PATCH] selinux: convert struct sidtab count to refcount_t

[Jann Horn]

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().

Re: [PATCH] selinux: convert struct sidtab count to refcount_t

[Kees Cook]

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

Re: [PATCH] selinux: convert struct sidtab count to refcount_t

[Jann Horn]

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.

Re: [PATCH] selinux: convert struct sidtab count to refcount_t

[Kees Cook]

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

Re: [PATCH] selinux: convert struct sidtab count to refcount_t

[Ondrej Mosnacek]

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.

Re: [PATCH] selinux: convert struct sidtab count to refcount_t

[Jann Horn]

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.

Re: [PATCH] selinux: convert struct sidtab count to refcount_t

[Kees Cook]

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