Re: [PATCH v2 3/9] selinux lsm IB/core: Implement LSM notification system

[Paul Moore <paul-r2n+y4ga6xFZroRs9YW3xA@public.gmane.org>]

On Thu, Jul 14, 2016 at 6:56 PM, Dan Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> From: Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>
> Add a generic notificaiton mechanism in the LSM. Interested consumers
> can register a callback with the LSM and security modules can produce
> events.
>
> Because access to Infiniband QPs are enforced in the setup phase of a
> connection security should be enforced again if the policy changes.
> Register infiniband devices for policy change notification and check all
> QPs on that device when the notification is received.
>
> Add a call to the notification mechanism is from SELinux when the AVC
> cache changes.
>
> Signed-off-by: Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>
> ---
> v2:
> - new patch that has the generic notification, replaces selinux and
>   IB/core patches related to the ib_flush callback. Yuval Shaia and Paul
>   Moore
> ---
>  drivers/infiniband/core/device.c |   46 ++++++++++++++++++++++++++++++
>  include/linux/security.h         |   10 ++++++
>  security/security.c              |   58 ++++++++++++++++++++++++++++++++++++++
>  security/selinux/hooks.c         |    5 ++-
>  security/selinux/selinuxfs.c     |    2 +
>  5 files changed, 119 insertions(+), 2 deletions(-)
>

Thanks for making this more generic.  I've got some comments below,
but in the course of reviewing this I realized that the kernel has an
existing, general purpose notifier mechanism (see
include/linux/notifier.h); while I've seen the notifier code in the
network stack, I never realized it was created as a general purpose
mechanism.  My apologies, I should have noticed this earlier and
mentioned it.

As far as how that impacts this patch; it seems like we should be good
citizens and use the general mechanism, I don't see any obvious
reasons why it wouldn't work.  That said, I do feel bad for not
realizing this earlier.  If it isn't too annoying, would you mind
updating this patch *again*?

> diff --git a/include/linux/security.h b/include/linux/security.h
> index 33e23c4..bf53911 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -69,6 +69,16 @@ struct audit_krule;
>  struct user_namespace;
>  struct timezone;
>
> +enum lsm_event {
> +       LSM_POLICY_CHANGE,
> +};
> +
> +typedef void (*lsm_notifier)(enum lsm_event event, void *ctx, void *data);
> +
> +void security_lsm_notify(enum lsm_event event, void *data);
> +int security_register_lsm_notifier(lsm_notifier func, void *ctx, u32 *id);
> +void security_unregister_lsm_notifier(u32 id);
> +
>  /* These functions are in security/commoncap.c */
>  extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
>                        int cap, int audit);
> diff --git a/security/security.c b/security/security.c
> index 234982d..1263c1d 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -33,6 +33,18 @@
>  /* Maximum number of letters for an LSM name string */
>  #define SECURITY_NAME_MAX      10
>
> +struct lsm_notifier_entry {
> +       u32                     callback_id;
> +       lsm_notifier            func;
> +       void                    *ctx;
> +       struct list_head        list;
> +       struct rcu_head         rcu;
> +};
> +
> +static LIST_HEAD(lsm_notifier_list);
> +static DEFINE_SPINLOCK(lsm_notifier_lock);
> +static u32 next_callback_id;

I'd rather see the callback ID named something that makes it obvious
it is tied to the lsm_notifier_* bits, perhaps lsm_notifier_nextid?

>  /* Boot-time LSM user choice */
>  static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
>         CONFIG_DEFAULT_SECURITY;
> @@ -98,6 +110,52 @@ int __init security_module_enable(const char *module)
>         return !strcmp(module, chosen_lsm);
>  }
>
> +void security_lsm_notify(enum lsm_event event, void *data)
> +{
> +       struct lsm_notifier_entry *entry;
> +
> +       rcu_read_lock();
> +       list_for_each_entry_rcu(entry, &lsm_notifier_list, list)
> +               entry->func(event, entry->ctx, data);
> +       rcu_read_unlock();
> +}
> +EXPORT_SYMBOL(security_lsm_notify);
> +
> +int security_register_lsm_notifier(lsm_notifier func, void *ctx, u32 *id)
> +{
> +       struct lsm_notifier_entry *entry;
> +
> +       entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> +       if (!entry)
> +               return -ENOMEM;
> +
> +       entry->func = func;
> +       entry->ctx = ctx;

Do we need ctx?  We do need to ability to pass a blob to the callback
which will vary based on the event, but I'm not sure the ctx here has
much value.

> +       spin_lock(&lsm_notifier_lock);
> +       entry->callback_id = next_callback_id++;
> +       *id = entry->callback_id;
> +       list_add_rcu(&entry->list, &lsm_notifier_list);
> +       spin_unlock(&lsm_notifier_lock);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(security_register_lsm_notifier);

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html