[PATCH v4 2/5] security: Count the LSMs enabled at compile time

[KP Singh]

On Fri, Sep 22, 2023 at 6:07 PM KP Singh <kpsingh at kernel.org> wrote:
>
> On Fri, Sep 22, 2023 at 5:50 PM Kees Cook <keescook at chromium.org> wrote:
> >
> > On Fri, Sep 22, 2023 at 04:55:02PM +0200, KP Singh wrote:
> > > These macros are a clever trick to determine a count of the number of
> > > LSMs that are enabled in the config to ascertain the maximum number of
> > > static calls that need to be configured per LSM hook.
> > >
> > > Without this one would need to generate static calls for the total
> > > number of LSMs in the kernel (even if they are not compiled) times the
> > > number of LSM hooks which ends up being quite wasteful.
> > >
> > > Suggested-by: Kui-Feng Lee <sinquersw at gmail.com>
> > > Suggested-by: Andrii Nakryiko <andrii at kernel.org>
> > > Acked-by: Song Liu <song at kernel.org>
> > > Signed-off-by: KP Singh <kpsingh at kernel.org>
> >
> > Thought below, but regardless of result:
> >
> > Reviewed-by: Kees Cook <keescook at chromium.org>
> >
> >
> > > ---
> > >  include/linux/lsm_count.h | 107 ++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 107 insertions(+)
> > >  create mode 100644 include/linux/lsm_count.h
> > >
> > > diff --git a/include/linux/lsm_count.h b/include/linux/lsm_count.h
> > > new file mode 100644
> > > index 000000000000..4d6dac6efb75
> > > --- /dev/null
> > > +++ b/include/linux/lsm_count.h
> > > @@ -0,0 +1,107 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +
> > > +/*
> > > + * Copyright (C) 2023 Google LLC.
> > > + */
> > > +
> > > +#ifndef __LINUX_LSM_COUNT_H
> > > +#define __LINUX_LSM_COUNT_H
> > > +
> > > +#include <linux/args.h>
> > > +
> > > +#ifdef CONFIG_SECURITY
> > > +
> > > +/*
> > > + * Macros to count the number of LSMs enabled in the kernel at compile time.
> > > + */
> > > +
> > > +/*
> > > + * Capabilities is enabled when CONFIG_SECURITY is enabled.
> > > + */
> > > +#if IS_ENABLED(CONFIG_SECURITY)
> > > +#define CAPABILITIES_ENABLED 1,
> > > +#else
> > > +#define CAPABILITIES_ENABLED
> > > +#endif
> >
> > We're in an #ifdef CONFIG_SECURITY, so CAPABILITIES_ENABLED will always
> > be set. As such, we could leave off the trailing comma and list it
> > _last_ in the macro, and then ...
> >
> > > +/*
> > > + *  There is a trailing comma that we need to be accounted for. This is done by
> > > + *  using a skipped argument in __COUNT_LSMS
> > > + */
> > > +#define __COUNT_LSMS(skipped_arg, args...) COUNT_ARGS(args)
> > > +#define COUNT_LSMS(args...) __COUNT_LSMS(args)
> >
> > This wouldn't be needed...
>
> Slight preference for explicitly having the capabilities listed than
> implicitly over counting. But no strong opinion, fine with either
> approches.

Actually it's not just a preference but really required. When the
CAPABILITIES is absent and all other LSMs are disabled it leads to
COUNT_ARGS() which evaluates to 0

This also happens here:

https://lore.kernel.org/bpf/202309271206.d7fb60f9-oliver.sang@intel.com/

and to fix this we need:

-#define __COUNT_LSMS(skipped_arg, args...) COUNT_ARGS(args)
+#define __COUNT_LSMS(skipped_arg, args...) COUNT_ARGS(args...)

And I checked the edge cases with a simple c file

int test(void) {
   int count = MAX_LSM_COUNT;
}

and make security/count.i:

for just CONFIG_SECURITY enabled:

int test(void) {
  int count = 1;
}

with another LSM:

int test(void) {
  int count = 2;
}


- KP
>
> >
> > > +
> > > +#define MAX_LSM_COUNT                        \
> > > +     COUNT_LSMS(                     \
> > > +             CAPABILITIES_ENABLED    \
> > > +             SELINUX_ENABLED         \
> > > +             SMACK_ENABLED           \
> > > +             APPARMOR_ENABLED        \
> > > +             TOMOYO_ENABLED          \
> > > +             YAMA_ENABLED            \
> > > +             LOADPIN_ENABLED         \
> > > +             LOCKDOWN_ENABLED        \
> > > +             BPF_LSM_ENABLED         \
> > > +             LANDLOCK_ENABLED)
> >
> >
> >         COUNT_ARGS(                     \
> >                 SELINUX_ENABLED         \
> >                 SMACK_ENABLED           \
> >                 ...
> >                 CAPABILITIES_ENABLED)
> >
> > -Kees
> >
> > --
> > Kees Cook