Message-ID: <3291294.XmtiFPecW4@sifl>
Date:	Thu, 06 Aug 2015 16:19:45 -0400
From:	Paul Moore <pmoore@...hat.com>
To:	Richard Guy Briggs <rgb@...hat.com>
Cc:	linux-audit@...hat.com, linux-kernel@...r.kernel.org,
	sgrubb@...hat.com, eparis@...hat.com, peter@...3.com
Subject: Re: [PATCH V9 1/3] audit: clean simple fsnotify implementation

On Wednesday, August 05, 2015 04:29:36 PM Richard Guy Briggs wrote:
> This is to be used to audit by executable path rules, but audit watches
> should be able to share this code eventually.
> 
> At the moment the audit watch code is a lot more complex.  That code only
> creates one fsnotify watch per parent directory.  That 'audit_parent' in
> turn has a list of 'audit_watches' which contain the name, ino, dev of
> the specific object we care about.  This just creates one fsnotify watch
> per object we care about.  So if you watch 100 inodes in /etc this code
> will create 100 fsnotify watches on /etc.  The audit_watch code will
> instead create 1 fsnotify watch on /etc (the audit_parent) and then 100
> individual watches chained from that fsnotify mark.
> 
> We should be able to convert the audit_watch code to do one fsnotify
> mark per watch and simplify things/remove a whole lot of code.  After
> that conversion we should be able to convert the audit_fsnotify code to
> support that hierarchy if the optimization is necessary.
> 
> Move the access to the entry for audit_match_signal() to the beginning of
> the audit_del_rule() function in case the entry found is the same one passed
> in.  This will enable it to be used by audit_autoremove_mark_rule(),
> kill_rules() and audit_remove_parent_watches().
> 
> This is a heavily modified and merged version of two patches originally
> submitted by Eric Paris.
> 
> Cc: Peter Moody <peter@...3.com>
> Cc: Eric Paris <eparis@...hat.com>
> Signed-off-by: Richard Guy Briggs <rgb@...hat.com>
> ---
>  kernel/Makefile         |    2 +-
>  kernel/audit.h          |   14 +++
>  kernel/audit_fsnotify.c |  215 +++++++++++++++++++++++++++++++++++++++++++
>  kernel/auditfilter.c    |    2 +-
>  4 files changed, 231 insertions(+), 2 deletions(-)
>  create mode 100644 kernel/audit_fsnotify.c

Merged, although I had to add a line of whitespace to keep checkpatch happy.

> diff --git a/kernel/Makefile b/kernel/Makefile
> index 60c302c..58b5b52 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -64,7 +64,7 @@ obj-$(CONFIG_SMP) += stop_machine.o
>  obj-$(CONFIG_KPROBES_SANITY_TEST) += test_kprobes.o
>  obj-$(CONFIG_AUDIT) += audit.o auditfilter.o
>  obj-$(CONFIG_AUDITSYSCALL) += auditsc.o
> -obj-$(CONFIG_AUDIT_WATCH) += audit_watch.o
> +obj-$(CONFIG_AUDIT_WATCH) += audit_watch.o audit_fsnotify.o
>  obj-$(CONFIG_AUDIT_TREE) += audit_tree.o
>  obj-$(CONFIG_GCOV_KERNEL) += gcov/
>  obj-$(CONFIG_KPROBES) += kprobes.o
> diff --git a/kernel/audit.h b/kernel/audit.h
> index d641f9b..46d10dd 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -50,6 +50,7 @@ enum audit_state {
> 
>  /* Rule lists */
>  struct audit_watch;
> +struct audit_fsnotify_mark;
>  struct audit_tree;
>  struct audit_chunk;
> 
> @@ -252,6 +253,7 @@ struct audit_net {
>  extern int selinux_audit_rule_update(void);
> 
>  extern struct mutex audit_filter_mutex;
> +extern int audit_del_rule(struct audit_entry *);
>  extern void audit_free_rule_rcu(struct rcu_head *);
>  extern struct list_head audit_filter_list[];
> 
> @@ -269,6 +271,13 @@ extern int audit_add_watch(struct audit_krule *krule,
> struct list_head **list); extern void audit_remove_watch_rule(struct
> audit_krule *krule);
>  extern char *audit_watch_path(struct audit_watch *watch);
>  extern int audit_watch_compare(struct audit_watch *watch, unsigned long
> ino, dev_t dev); +
> +extern struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule
> *krule, char *pathname, int len); +extern char *audit_mark_path(struct
> audit_fsnotify_mark *mark);
> +extern void audit_remove_mark(struct audit_fsnotify_mark *audit_mark);
> +extern void audit_remove_mark_rule(struct audit_krule *krule);
> +extern int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned
> long ino, dev_t dev); +
>  #else
>  #define audit_put_watch(w) {}
>  #define audit_get_watch(w) {}
> @@ -278,6 +287,11 @@ extern int audit_watch_compare(struct audit_watch
> *watch, unsigned long ino, dev #define audit_watch_path(w) ""
>  #define audit_watch_compare(w, i, d) 0
> 
> +#define audit_alloc_mark(k, p, l) (ERR_PTR(-EINVAL))
> +#define audit_mark_path(m) ""
> +#define audit_remove_mark(m)
> +#define audit_remove_mark_rule(k)
> +#define audit_mark_compare(m, i, d) 0
>  #endif /* CONFIG_AUDIT_WATCH */
> 
>  #ifdef CONFIG_AUDIT_TREE
> diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
> new file mode 100644
> index 0000000..654c6c7
> --- /dev/null
> +++ b/kernel/audit_fsnotify.c
> @@ -0,0 +1,215 @@
> +/* audit_fsnotify.c -- tracking inodes
> + *
> + * Copyright 2003-2009,2014-2015 Red Hat, Inc.
> + * Copyright 2005 Hewlett-Packard Development Company, L.P.
> + * Copyright 2005 IBM Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/audit.h>
> +#include <linux/kthread.h>
> +#include <linux/mutex.h>
> +#include <linux/fs.h>
> +#include <linux/fsnotify_backend.h>
> +#include <linux/namei.h>
> +#include <linux/netlink.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/security.h>
> +#include "audit.h"
> +
> +/*
> + * this mark lives on the parent directory of the inode in question.
> + * but dev, ino, and path are about the child
> + */
> +struct audit_fsnotify_mark {
> +	dev_t dev;		/* associated superblock device */
> +	unsigned long ino;	/* associated inode number */
> +	char *path;		/* insertion path */
> +	struct fsnotify_mark mark; /* fsnotify mark on the inode */
> +	struct audit_krule *rule;
> +};
> +
> +/* fsnotify handle. */
> +static struct fsnotify_group *audit_fsnotify_group;
> +
> +/* fsnotify events we care about. */
> +#define AUDIT_FS_EVENTS (FS_MOVE | FS_CREATE | FS_DELETE | FS_DELETE_SELF
> |\ +			 FS_MOVE_SELF | FS_EVENT_ON_CHILD)
> +
> +static void audit_fsnotify_mark_free(struct audit_fsnotify_mark
> *audit_mark) +{
> +	kfree(audit_mark->path);
> +	kfree(audit_mark);
> +}
> +
> +static void audit_fsnotify_free_mark(struct fsnotify_mark *mark)
> +{
> +	struct audit_fsnotify_mark *audit_mark;
> +
> +	audit_mark = container_of(mark, struct audit_fsnotify_mark, mark);
> +	audit_fsnotify_mark_free(audit_mark);
> +}
> +
> +char *audit_mark_path(struct audit_fsnotify_mark *mark)
> +{
> +	return mark->path;
> +}
> +
> +int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned long ino,
> dev_t dev) +{
> +	if (mark->ino == AUDIT_INO_UNSET)
> +		return 0;
> +	return (mark->ino == ino) && (mark->dev == dev);
> +}
> +
> +static void audit_update_mark(struct audit_fsnotify_mark *audit_mark,
> +			     struct inode *inode)
> +{
> +	audit_mark->dev = inode ? inode->i_sb->s_dev : AUDIT_DEV_UNSET;
> +	audit_mark->ino = inode ? inode->i_ino : AUDIT_INO_UNSET;
> +}
> +
> +struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule,
> char *pathname, int len) +{
> +	struct audit_fsnotify_mark *audit_mark;
> +	struct path path;
> +	struct dentry *dentry;
> +	struct inode *inode;
> +	int ret;
> +
> +	if (pathname[0] != '/' || pathname[len-1] == '/')
> +		return ERR_PTR(-EINVAL);
> +
> +	dentry = kern_path_locked(pathname, &path);
> +	if (IS_ERR(dentry))
> +		return (void *)dentry; /* returning an error */
> +	inode = path.dentry->d_inode;
> +	mutex_unlock(&inode->i_mutex);
> +
> +	audit_mark = kzalloc(sizeof(*audit_mark), GFP_KERNEL);
> +	if (unlikely(!audit_mark)) {
> +		audit_mark = ERR_PTR(-ENOMEM);
> +		goto out;
> +	}
> +
> +	fsnotify_init_mark(&audit_mark->mark, audit_fsnotify_free_mark);
> +	audit_mark->mark.mask = AUDIT_FS_EVENTS;
> +	audit_mark->path = pathname;
> +	audit_update_mark(audit_mark, dentry->d_inode);
> +	audit_mark->rule = krule;
> +
> +	ret = fsnotify_add_mark(&audit_mark->mark, audit_fsnotify_group, inode,
> NULL, true); +	if (ret < 0) {
> +		audit_fsnotify_mark_free(audit_mark);
> +		audit_mark = ERR_PTR(ret);
> +	}
> +out:
> +	dput(dentry);
> +	path_put(&path);
> +	return audit_mark;
> +}
> +
> +static void audit_mark_log_rule_change(struct audit_fsnotify_mark
> *audit_mark, char *op) +{
> +	struct audit_buffer *ab;
> +	struct audit_krule *rule = audit_mark->rule;
> +	if (!audit_enabled)
> +		return;
> +	ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> +	if (unlikely(!ab))
> +		return;
> +	audit_log_format(ab, "auid=%u ses=%u op=",
> +			 from_kuid(&init_user_ns, audit_get_loginuid(current)),
> +			 audit_get_sessionid(current));
> +	audit_log_string(ab, op);
> +	audit_log_format(ab, " path=");
> +	audit_log_untrustedstring(ab, audit_mark->path);
> +	audit_log_key(ab, rule->filterkey);
> +	audit_log_format(ab, " list=%d res=1", rule->listnr);
> +	audit_log_end(ab);
> +}
> +
> +void audit_remove_mark(struct audit_fsnotify_mark *audit_mark)
> +{
> +	fsnotify_destroy_mark(&audit_mark->mark, audit_fsnotify_group);
> +	fsnotify_put_mark(&audit_mark->mark);
> +}
> +
> +void audit_remove_mark_rule(struct audit_krule *krule)
> +{
> +	struct audit_fsnotify_mark *mark = krule->exe;
> +
> +	audit_remove_mark(mark);
> +}
> +
> +static void audit_autoremove_mark_rule(struct audit_fsnotify_mark
> *audit_mark) +{
> +	struct audit_krule *rule = audit_mark->rule;
> +	struct audit_entry *entry = container_of(rule, struct audit_entry, rule);
> +
> +	audit_mark_log_rule_change(audit_mark, "autoremove_rule");
> +	audit_del_rule(entry);
> +}
> +
> +/* Update mark data in audit rules based on fsnotify events. */
> +static int audit_mark_handle_event(struct fsnotify_group *group,
> +				    struct inode *to_tell,
> +				    struct fsnotify_mark *inode_mark,
> +				    struct fsnotify_mark *vfsmount_mark,
> +				    u32 mask, void *data, int data_type,
> +				    const unsigned char *dname, u32 cookie)
> +{
> +	struct audit_fsnotify_mark *audit_mark;
> +	struct inode *inode = NULL;
> +
> +	audit_mark = container_of(inode_mark, struct audit_fsnotify_mark, mark);
> +
> +	BUG_ON(group != audit_fsnotify_group);
> +
> +	switch (data_type) {
> +	case (FSNOTIFY_EVENT_PATH):
> +		inode = ((struct path *)data)->dentry->d_inode;
> +		break;
> +	case (FSNOTIFY_EVENT_INODE):
> +		inode = (struct inode *)data;
> +		break;
> +	default:
> +		BUG();
> +		return 0;
> +	};
> +
> +	if (mask & (FS_CREATE|FS_MOVED_TO|FS_DELETE|FS_MOVED_FROM)) {
> +		if (audit_compare_dname_path(dname, audit_mark->path, AUDIT_NAME_FULL))
> +			return 0;
> +		audit_update_mark(audit_mark, inode);
> +	} else if (mask & (FS_DELETE_SELF|FS_UNMOUNT|FS_MOVE_SELF))
> +		audit_autoremove_mark_rule(audit_mark);
> +
> +	return 0;
> +}
> +
> +static const struct fsnotify_ops audit_mark_fsnotify_ops = {
> +	.handle_event =	audit_mark_handle_event,
> +};
> +
> +static int __init audit_fsnotify_init(void)
> +{
> +	audit_fsnotify_group = fsnotify_alloc_group(&audit_mark_fsnotify_ops);
> +	if (IS_ERR(audit_fsnotify_group)) {
> +		audit_fsnotify_group = NULL;
> +		audit_panic("cannot create audit fsnotify group");
> +	}
> +	return 0;
> +}
> +device_initcall(audit_fsnotify_init);
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 018719a..3d99196 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -935,7 +935,7 @@ static inline int audit_add_rule(struct audit_entry
> *entry) }
> 
>  /* Remove an existing rule from filterlist. */
> -static inline int audit_del_rule(struct audit_entry *entry)
> +int audit_del_rule(struct audit_entry *entry)
>  {
>  	struct audit_entry  *e;
>  	struct audit_tree *tree = entry->rule.tree;

-- 
paul moore
security @ redhat

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists