On Thu, Jul 14, 2016 at 6:56 PM, Dan Jurgens <danielj@xxxxxxxxxxxx> wrote:
> From: Daniel Jurgens <danielj@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
>
> ---
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
