Re: [PATCH v3 02/14] selinux: use u16 for security classes

[David Laight <david.laight.linux@gmail.com>]

On Tue, 13 May 2025 15:44:09 -0400
Stephen Smalley <stephen.smalley.work@gmail.com> wrote:

> On Sun, May 11, 2025 at 1:31 PM Christian Göttsche
> <cgoettsche@seltendoof.de> wrote:
> >
> > From: Christian Göttsche <cgzones@googlemail.com>
> >
> > Security class identifiers are limited to 2^16, thus use the appropriate
> > type u16 consistently.
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>  
> 
> Historical footnote: originally security classes were _not_ limited to
> 2^16 but a later memory optimization of the avtab rendered them so.

The fact that the value is limited to 2^16 doesn't mean that the type
can't be larger.
You won't save anything by using u16 instead of u32, if anything the
code will get larger and there can be subtle changes because u16 gets
promoted to 'signed int' before being used.

	David

> 
> Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> 
> > ---
> > v3: only change type, move the validation (> U16_MAX) to the subsequent
> >     patch
> > ---
> >  security/selinux/ss/policydb.c |  5 +++--
> >  security/selinux/ss/policydb.h | 10 +++++-----
> >  security/selinux/ss/services.c |  2 +-
> >  3 files changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> > index dc701a7f8652..f490556ddb5c 100644
> > --- a/security/selinux/ss/policydb.c
> > +++ b/security/selinux/ss/policydb.c
> > @@ -927,7 +927,7 @@ int policydb_load_isids(struct policydb *p, struct sidtab *s)
> >         return 0;
> >  }
> >
> > -int policydb_class_isvalid(struct policydb *p, unsigned int class)
> > +int policydb_class_isvalid(struct policydb *p, u16 class)
> >  {
> >         if (!class || class > p->p_classes.nprim)
> >                 return 0;
> > @@ -2003,7 +2003,8 @@ static int filename_trans_read_helper(struct policydb *p, struct policy_file *fp
> >         struct filename_trans_key *ft = NULL;
> >         struct filename_trans_datum **dst, *datum, *first = NULL;
> >         char *name = NULL;
> > -       u32 len, ttype, tclass, ndatum, i;
> > +       u32 len, ttype, ndatum, i;
> > +       u16 tclass;
> >         __le32 buf[3];
> >         int rc;
> >
> > diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
> > index 25650224b6e7..0c423ad77fd9 100644
> > --- a/security/selinux/ss/policydb.h
> > +++ b/security/selinux/ss/policydb.h
> > @@ -48,7 +48,7 @@ struct common_datum {
> >
> >  /* Class attributes */
> >  struct class_datum {
> > -       u32 value; /* class value */
> > +       u16 value; /* class value */
> >         char *comkey; /* common name */
> >         struct common_datum *comdatum; /* common datum */
> >         struct symtab permissions; /* class-specific permission symbol table */
> > @@ -82,7 +82,7 @@ struct role_datum {
> >  struct role_trans_key {
> >         u32 role; /* current role */
> >         u32 type; /* program executable type, or new object type */
> > -       u32 tclass; /* process class, or new object class */
> > +       u16 tclass; /* process class, or new object class */
> >  };
> >
> >  struct role_trans_datum {
> > @@ -139,7 +139,7 @@ struct cat_datum {
> >  struct range_trans {
> >         u32 source_type;
> >         u32 target_type;
> > -       u32 target_class;
> > +       u16 target_class;
> >  };
> >
> >  /* Boolean data type */
> > @@ -195,7 +195,7 @@ struct ocontext {
> >                 } ibendport;
> >         } u;
> >         union {
> > -               u32 sclass; /* security class for genfs */
> > +               u16 sclass; /* security class for genfs */
> >                 u32 behavior; /* labeling behavior for fs_use */
> >         } v;
> >         struct context context[2]; /* security context(s) */
> > @@ -320,7 +320,7 @@ struct policy_file {
> >  extern void policydb_destroy(struct policydb *p);
> >  extern int policydb_load_isids(struct policydb *p, struct sidtab *s);
> >  extern int policydb_context_isvalid(struct policydb *p, struct context *c);
> > -extern int policydb_class_isvalid(struct policydb *p, unsigned int class);
> > +extern int policydb_class_isvalid(struct policydb *p, u16 class);
> >  extern int policydb_type_isvalid(struct policydb *p, unsigned int type);
> >  extern int policydb_role_isvalid(struct policydb *p, unsigned int role);
> >  extern int policydb_read(struct policydb *p, struct policy_file *fp);
> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > index 7becf3808818..a2dd42e750fe 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -3387,7 +3387,7 @@ static int get_classes_callback(void *k, void *d, void *args)
> >  {
> >         struct class_datum *datum = d;
> >         char *name = k, **classes = args;
> > -       u32 value = datum->value - 1;
> > +       u16 value = datum->value - 1;
> >
> >         classes[value] = kstrdup(name, GFP_ATOMIC);
> >         if (!classes[value])
> > --
> > 2.49.0
> >  
>