[PATCH] filesystem location based auditing

[Amy Griffis]

On Fri, Feb 24, 2006 at 03:07:30PM -0600, Timothy R. Chavez wrote:
> > +/* Inotify events we care about. */
> > +#define AUDIT_IN_WATCH IN_MOVE|IN_CREATE|IN_DELETE|IN_DELETE_SELF|IN_MOVE_SELF
> > +#define AUDIT_IN_SELF  IN_DELETE_SELF|IN_MOVE_SELF|IN_UNMOUNT
> > +
> > +/* Flags for stale filterlist data */
> > +#define AUDIT_ENTRY_ADD  0x01	/* Rule entry addition in progress. */
> > +#define AUDIT_ENTRY_DEL  0x02	/* Rule entry deletion in progress. */
> > +#define AUDIT_PARENT_DEL 0x01	/* Parent deletion in progress. */
> 
> AUDIT_ENTRY_ADD and AUDIT_PARENT_DEL are the same?  I'm assuming since
> this is a mask, that it probably makes sense to make 0x04, no?

AUDIT_ENTRY_* and AUDIT_PARENT_* were two different masks in the
audit_entry and audit_parent structs.  I've re-written the code
though, so neither of them is needed now.

> > +/* Remove all watches & rules associated with a parent that is going away. */
> > +static inline void audit_remove_parent_watches(struct audit_parent *parent)
> > +{
> > +	struct audit_watch *w, *nextw;
> > +	struct audit_krule *r, *nextr;
> > +	struct audit_entry *e;
> > +	struct audit_flist *flist = &audit_filter_list[AUDIT_FILTER_EXIT];
> > +
> > +	spin_lock(&flist->lock);
> > +	list_for_each_entry_safe(w, nextw, &parent->watches, wlist) {
> > +		list_for_each_entry_safe(r, nextr, &w->rules, rlist) {
> > +			e = container_of(r, struct audit_entry, rule);
> > +
> > +			/* make sure we have a valid copy */
> > +			while (e->replacement != NULL)
> > +				e = e->replacement;
> > +			if (e->flags & AUDIT_ENTRY_DEL)
> > +				continue;
> > +
> > +			list_del(&r->rlist);
> > +			list_del_rcu(&e->list);
> > +			e->flags |= AUDIT_ENTRY_DEL;
> > +			call_rcu(&e->rcu, audit_free_rule_rcu);
> > +			audit_log(NULL, GFP_ATOMIC, AUDIT_CONFIG_CHANGE,
> > +				 "audit implicitly removed rule from list=%d\n",
> > +				  AUDIT_FILTER_EXIT);
> > +		}
> > +		list_del(&w->wlist);
> > +		audit_put_watch(w);
> > +	}
> > +	spin_unlock(&flist->lock);
> > +}
> 
> Why not rcu locking here?  You can iterate over the list w/
> rcu_read_lock/unlock and then removals from the list via admin
> action can be defferred using list_del_rcu?  Doesn't the same apply
> for additions to the flist as well.. 

I am using rcu locking, but you need an additional lock to protect
against concurrent list writes.

> > +/* Register this parent's inotify watch. */
> > +static int audit_inotify_register(void *_data)
> > +{
> > +	void **data = _data;
> > +	struct audit_parent *parent;
> > +	char *path;
> > +	struct nameidata nd;
> > +	int err;
> > +	u32 wd;
> > +
> > +	parent = data[0];
> > +	path = data[1];
> > +	kfree(data);
> > +
> > +	err = path_lookup(path, LOOKUP_PARENT, &nd);
> > +	if (err)
> > +		goto handle_error;
> > +
> > +	wd = inotify_add_watch(audit_idev, nd.dentry->d_inode, AUDIT_IN_WATCH,
> > +			       parent);
> > +	if (wd < 0)
> > +		goto handle_error;
> > +
> > +	spin_lock(&master_parents_lock);
> > +	parent->wd = wd;
> > +	parent->ino = nd.dentry->d_inode->i_ino;
> > +	spin_unlock(&master_parents_lock);
> > +
> > +	path_release(&nd);
> > +	audit_put_parent(parent);
> > +	return 0;
> > +
> > +handle_error:
> > +	path_release(&nd);
> > +	audit_remove_parent_watches(parent);
> > +	audit_remove_parent(parent);
> > +
> > +	audit_put_parent(parent);
> > +	return 0;
> 
> Hm... on error you return 0?  That's what you return on success too.

Yeah, the function was supposed to contain the error by cleaning up
after itself.  It's a little different in the latest iteration...

> > +/* Unregister this parent's inotify watch.  Generates an IN_IGNORED event. */
> > +static int audit_inotify_unregister(void *data)
> > +{
> > +	struct audit_parent *parent = data;
> > +	s32 wd;
> > +
> > +	spin_lock(&master_parents_lock);
> > +	wd = parent->wd;
> > +	spin_unlock(&master_parents_lock);
> > +
> > +	if (inotify_ignore(audit_idev, wd))
> > +		printk(KERN_ERR
> > +		       "%s:%d: unable to remove inotify watch for inode %lu\n",
> > +		       __FILE__, __LINE__, parent->ino);
> 
> This is benign?

Not really benign, but a way to signal an un-recoverable error.  This
situation has been removed by not using kthread_run.

> > +/* Initialize a parent watch entry. */
> > +static inline struct audit_parent *audit_init_parent(char *path,
> > +						     unsigned long ino)
> > +{
> > +	struct audit_parent *parent;
> > +	void **data;
> > +	struct task_struct *task;
> > +
> > +	parent = kmalloc(sizeof(*parent), GFP_ATOMIC);
> > +	if (unlikely(!parent))
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	memset(parent, 0, sizeof(*parent));
> > +	INIT_LIST_HEAD(&parent->watches);
> > +	atomic_set(&parent->count, 0);
> > +	parent->ino = ino;
> > +	audit_get_parent(parent);
> > +
> > +	/* Spawn a thread to register the parent's inotify watch without
> > +	 * the filterlist spinlock. */
> > +	data = kmalloc(2 * sizeof(void *), GFP_ATOMIC);
> > +	if (!data) {
> > +		audit_put_parent(parent);
> > +		return ERR_PTR(-ENOMEM);
> > +	}
> > +	data[0] = parent;
> > +	data[1] = path;
> > +	audit_get_parent(parent);
> > +	task = kthread_run(audit_inotify_register, data,
> > +			   "audit_inotify_register");
> > +	if (IS_ERR(task)) {
> > +		audit_put_parent(parent);
> > +		return ERR_PTR(PTR_ERR(task));
> 
> Redundant?  return ERR_PTR(task), right?

The function returns an audit_parent struct, and this is a task
struct.  Anyway, it's been removed as a result of other changes. :-)

> > +/* Initialize a watch entry. */
> > +static inline struct audit_watch *audit_init_watch(char *path)
> > +{
> > +	struct audit_watch *watch;
> > +
> > +	watch = kmalloc(sizeof(*watch), GFP_KERNEL);
> > +	if (unlikely(!watch))
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	memset(watch, 0, sizeof(*watch));
> > +	INIT_LIST_HEAD(&watch->rules);
> > +	atomic_set(&watch->count, 0);
> > +	watch->path = path;
> > +	audit_get_watch(watch);
> > +
> 
> Why not just atomic_set(&watch->count, 1), rather than making two
> atomic calls?

Done.

> > +/* Compare given dentry name with last component in given path,
> > + * return of 0 indicates a match. */
> > +int audit_compare_dname_path(const char *dname, const char *path)
> > +{
> > +	int dlen, plen;
> > +	const char *p;
> >  
> > +	if (!dname || !path)
> > +		return 1;
> > +
> > +	dlen = strlen(dname);
> > +	plen = strlen(path);
> > +	if (plen < dlen)
> > +		return 1;
> > +
> > +	/* disregard trailing slashes */
> > +	p = path + plen - 1;
> > +	while ((*p == '/') && (p > path))
> > +		p--;
> > +
> > +	/* find last path component */
> 
> Why not path_lookup with LOOKUP_PARENT, then d_lookup?

I was hoping to avoid doing path_lookup here.

> > +	p = p - dlen + 1;
> > +	if (p < path)
> > +		return 1;
> > +	else if (p > path) {
> > +		if (*--p != '/')
> > +			return 1;
> > +		else
> > +			p++;
> > +	}
> > +
> > +	return strncmp(p, dname, dlen);
> > +}