[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH] filesystem location based auditing



On Fri, 2006-02-24 at 15:19 -0500, Amy Griffis wrote: 
> Hello,
> 
> This patch provides the functionality which allows a user to specify
> audit rules targeting specific filesystem locations.  It is an update
> of the previously posted patch which provided functionality solely for
> adding/removing rules with AUDIT_WATCH fields:
<snip>
> Regards,
> Amy
> 

Hi Amy,

Just took a cursory glance and noted a few little things.

-tim

<snip>
> +/* 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?

[..] 
> +
> +static inline void audit_get_parent(struct audit_parent *parent)
> +{
> +	atomic_inc(&parent->count);
> +}
> +
> +static inline void audit_put_parent(struct audit_parent *parent)
> +{
> +	if (atomic_dec_and_test(&parent->count)) {
> +		BUG_ON(!list_empty(&parent->watches));
> +		kfree(parent);
> +	}
> +}
> +
> +static inline void audit_get_watch(struct audit_watch *watch)
> +{
> +	atomic_inc(&watch->count);
> +}
> +
> +static inline void audit_put_watch(struct audit_watch *watch)
> +{
> +	if (atomic_dec_and_test(&watch->count)) {
> +		BUG_ON(!list_empty(&watch->rules));
> +		/* watches that were never added don't have a parent */
> +		if (watch->parent)
> +			audit_put_parent(watch->parent);
> +		kfree(watch->path);
> +		kfree(watch);
> +	}
> +}
>  
>  static inline void audit_free_rule(struct audit_entry *e)
>  {
> +	/* handle rules that don't have associated watches */
> +	if (e->rule.watch)
> +		audit_put_watch(e->rule.watch);
>  	kfree(e->rule.fields);
>  	kfree(e);
>  }
> @@ -52,6 +91,190 @@ static inline void audit_free_rule_rcu(s
>  	audit_free_rule(e);
>  }
>  
> +/* 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.. 

Admittedly, I haven't looked extensively at the code though :/

[..]
> +
> +/* Actually remove the parent; inotify has acknowleged the removal. */
> +static inline void audit_remove_parent(struct audit_parent *parent)
> +{
> +	BUG_ON(!list_empty(&parent->watches));
> +	spin_lock(&master_parents_lock);
> +	list_del(&parent->mlist);
> +	audit_put_parent(parent);
> +	spin_unlock(&master_parents_lock);
> +}
> +
> +
> +/* 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.

[..]
> +}
> +
> +/* 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?

[..]
> +	audit_put_parent(parent);
> +	return 0;
> +}
> +
> +/* 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?

[..]
> +	}
> +
> +	return parent;
> +}
> +
> +/* 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?

[..]
> +	return watch;
> +}
> +
> +/* Initialize an audit filterlist entry. */
> +static inline struct audit_entry *audit_init_entry(u32 field_count,
> +						   gfp_t gfp_mask)
> +{
> +	struct audit_entry *entry;
> +	struct audit_field *fields;
> +
> +	entry = kmalloc(sizeof(*entry), gfp_mask);
> +	if (unlikely(!entry))
> +		return NULL;
> +
> +	fields = kmalloc(sizeof(*fields) * field_count, gfp_mask);
> +	if (unlikely(!fields)) {
> +		kfree(entry);
> +		return NULL;
> +	}
> +
> +	memset(entry, 0, sizeof(struct audit_entry));
> +	memset(fields, 0, sizeof(struct audit_field) * field_count);
> +
> +	entry->rule.fields = fields;
> +
> +	return entry;
> +}
> +
>  /* Unpack a filter field's string representation from user-space
>   * buffer. */
>  static char *audit_unpack_string(void **bufp, size_t *remain, size_t len)
> @@ -79,12 +302,33 @@ static char *audit_unpack_string(void **
>  	return str;
>  }
>  
> +/* Translate a watch string to kernel respresentation. */
> +static int audit_to_watch(char *path, struct audit_krule *krule, int fidx)
> +{
> +	struct audit_field *f = &krule->fields[fidx];
> +	struct audit_watch *watch;
> +
> +	if (path[0] != '/' || path[f->val-1] == '/' ||
> +	    krule->listnr != AUDIT_FILTER_EXIT ||
> +	    f->op & ~AUDIT_EQUAL)
> +		return -EINVAL;
> +
> +	watch = audit_init_watch(path);
> +	if (unlikely(IS_ERR(watch)))
> +		return PTR_ERR(watch);
> +
> +	audit_get_watch(watch);
> +	krule->watch = watch;
> +	f->val = (unsigned int)-1;
> +
> +	return 0;
> +}
> +
>  /* Common user-space to kernel rule translation. */
>  static inline struct audit_entry *audit_to_entry_common(struct audit_rule *rule)
>  {
>  	unsigned listnr;
>  	struct audit_entry *entry;
> -	struct audit_field *fields;
>  	int i, err;
>  
>  	err = -EINVAL;
> @@ -108,23 +352,14 @@ static inline struct audit_entry *audit_
>  		goto exit_err;
>  
>  	err = -ENOMEM;
> -	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> -	if (unlikely(!entry))
> +	entry = audit_init_entry(rule->field_count, GFP_KERNEL);
> +	if (!entry)
>  		goto exit_err;
> -	fields = kmalloc(sizeof(*fields) * rule->field_count, GFP_KERNEL);
> -	if (unlikely(!fields)) {
> -		kfree(entry);
> -		goto exit_err;
> -	}
> -
> -	memset(&entry->rule, 0, sizeof(struct audit_krule));
> -	memset(fields, 0, sizeof(struct audit_field));
>  
>  	entry->rule.flags = rule->flags & AUDIT_FILTER_PREPEND;
>  	entry->rule.listnr = listnr;
>  	entry->rule.action = rule->action;
>  	entry->rule.field_count = rule->field_count;
> -	entry->rule.fields = fields;
>  
>  	for (i = 0; i < AUDIT_BITMASK_SIZE; i++)
>  		entry->rule.mask[i] = rule->mask[i];
> @@ -150,15 +385,16 @@ static struct audit_entry *audit_rule_to
>  	for (i = 0; i < rule->field_count; i++) {
>  		struct audit_field *f = &entry->rule.fields[i];
>  
> -		if (rule->fields[i] & AUDIT_UNUSED_BITS) {
> -			err = -EINVAL;
> -			goto exit_free;
> -		}
> -
>  		f->op = rule->fields[i] & (AUDIT_NEGATE|AUDIT_OPERATORS);
>  		f->type = rule->fields[i] & ~(AUDIT_NEGATE|AUDIT_OPERATORS);
>  		f->val = rule->values[i];
>  
> +		if (f->type & AUDIT_UNUSED_BITS ||
> +		    f->type == AUDIT_WATCH) {
> +			err = -EINVAL;
> +			goto exit_free;
> +		}
> +
>  		entry->rule.vers_ops = (f->op & AUDIT_OPERATORS) ? 2 : 1;
>  		if (f->op & AUDIT_NEGATE)
>  			f->op |= AUDIT_NOT_EQUAL;
> @@ -182,8 +418,9 @@ static struct audit_entry *audit_data_to
>  	int err = 0;
>  	struct audit_entry *entry;
>  	void *bufp;
> -	/* size_t remain = datasz - sizeof(struct audit_rule_data); */
> +	size_t remain = datasz - sizeof(struct audit_rule_data);
>  	int i;
> +	char *path;
>  
>  	entry = audit_to_entry_common((struct audit_rule *)data);
>  	if (IS_ERR(entry))
> @@ -201,10 +438,20 @@ static struct audit_entry *audit_data_to
>  
>  		f->op = data->fieldflags[i] & AUDIT_OPERATORS;
>  		f->type = data->fields[i];
> +		f->val = data->values[i];
>  		switch(f->type) {
> -		/* call type-specific conversion routines here */
> -		default:
> -			f->val = data->values[i];
> +		case AUDIT_WATCH:
> +			path = audit_unpack_string(&bufp, &remain, f->val);
> +			if (IS_ERR(path))
> +				goto exit_free;
> +			entry->rule.buflen += f->val;
> +
> +			err = audit_to_watch(path, &entry->rule, i);
> +			if (err) {
> +				kfree(path);
> +				goto exit_free;
> +			}
> +			break;
>  		}
>  	}
>  
> @@ -234,7 +481,8 @@ static struct audit_rule *audit_krule_to
>  	struct audit_rule *rule;
>  	int i;
>  
> -	rule = kmalloc(sizeof(*rule), GFP_KERNEL);
> +	/* use GFP_ATOMIC because we're under rcu_read_lock() */
> +	rule = kmalloc(sizeof(*rule), GFP_ATOMIC);
>  	if (unlikely(!rule))
>  		return ERR_PTR(-ENOMEM);
>  	memset(rule, 0, sizeof(*rule));
> @@ -265,7 +513,8 @@ static struct audit_rule_data *audit_kru
>  	void *bufp;
>  	int i;
>  
> -	data = kmalloc(sizeof(*data) + krule->buflen, GFP_KERNEL);
> +	/* use GFP_ATOMIC because we're under rcu_read_lock() */
> +	data = kmalloc(sizeof(*data) + krule->buflen, GFP_ATOMIC);
>  	if (unlikely(!data))
>  		return ERR_PTR(-ENOMEM);
>  	memset(data, 0, sizeof(*data));
> @@ -280,7 +529,10 @@ static struct audit_rule_data *audit_kru
>  		data->fields[i] = f->type;
>  		data->fieldflags[i] = f->op;
>  		switch(f->type) {
> -		/* call type-specific conversion routines here */
> +		case AUDIT_WATCH:
> +			data->buflen += data->values[i] =
> +				audit_pack_string(&bufp, krule->watch->path);
> +			break;
>  		default:
>  			data->values[i] = f->val;
>  		}
> @@ -290,6 +542,12 @@ static struct audit_rule_data *audit_kru
>  	return data;
>  }
>  
> +/* Compare two watches.  Considered success if rules don't match. */
> +static inline int audit_compare_watch(struct audit_watch *a, struct audit_watch *b)
> +{
> +	return strcmp(a->path, b->path);
> +}
> +
>  /* Compare two rules in kernel format.  Considered success if rules
>   * don't match. */
>  static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)
> @@ -308,7 +566,10 @@ static int audit_compare_rule(struct aud
>  			return 1;
>  
>  		switch(a->fields[i].type) {
> -		/* call type-specific comparison routines here */
> +		case AUDIT_WATCH:
> +			if (audit_compare_watch(a->watch, b->watch))
> +				return 1;
> +			break;
>  		default:
>  			if (a->fields[i].val != b->fields[i].val)
>  				return 1;
> @@ -322,45 +583,264 @@ static int audit_compare_rule(struct aud
>  	return 0;
>  }
>  
> +/* Copy an audit rule entry to be replaced.
> + * Caller must hold filterlist lock. */
> +static inline struct audit_entry *audit_dupe_rule(struct audit_entry *old)
> +{
> +	u32 fcount = old->rule.field_count;
> +	struct audit_entry *new;
> +	struct audit_field *fields;
> +	struct audit_watch *watch;
> +
> +	new = audit_init_entry(fcount, GFP_ATOMIC);
> +	if (unlikely(!new))
> +		return ERR_PTR(-ENOMEM);
> +
> +	fields = new->rule.fields;
> +	memcpy(&new->rule, &old->rule, sizeof(struct audit_krule));
> +	memcpy(fields, old->rule.fields, sizeof(struct audit_field) * fcount);
> +	new->rule.fields = fields;
> +
> +	watch = old->rule.watch;
> +	audit_get_watch(watch);
> +	list_add(&new->rule.rlist, &watch->rules);
> +	list_del(&old->rule.rlist);
> +
> +	return new;
> +}
> +
> +/* Update an audit rule field.  If the rule is part of a filterlist, caller
> + * must hold that filterlist's lock. */
> +static void audit_update_rule(struct audit_krule *krule, u32 type, u32 val)
> +{
> +	int i;
> +	struct audit_entry *old, *new;
> +
> +	for (i = 0; i < AUDIT_MAX_FIELDS; i++) {
> +		if (krule->fields[i].type != type)
> +			continue;
> +
> +		old = container_of(krule, struct audit_entry, rule);
> +
> +		/* rule is not in filterlist yet */
> +		if (old->flags & AUDIT_ENTRY_ADD) {
> +			krule->fields[i].val = val;
> +			return;
> +		}
> +
> +		/* make sure we have a valid copy */
> +		while (old->replacement != NULL)
> +			old = old->replacement;
> +		if (old->flags & AUDIT_ENTRY_DEL)
> +			return;
> +
> +		new = audit_dupe_rule(old);
> +		if (unlikely(IS_ERR(new))) {
> +			audit_panic("cannot allocate memory for rule update");
> +			return;
> +		}
> +		new->rule.fields[i].val = val;
> +
> +		old->flags |= AUDIT_ENTRY_DEL;
> +		old->replacement = new;
> +		list_replace_rcu(&old->list, &new->list);
> +		call_rcu(&old->rcu, audit_free_rule_rcu);
> +	}
> +}
> +
> +/* Find an existing parent entry for this watch, or create a new one.
> + * Caller must hold exit filterlist lock. */
> +static inline struct audit_parent *audit_find_parent(char *path)
> +{
> +	int err;
> +	struct nameidata nd;
> +	struct audit_parent *p, *parent;
> +	unsigned long ino;
> +
> +	err = path_lookup(path, LOOKUP_PARENT, &nd);
> +	if (err) {
> +		path_release(&nd);
> +		parent = ERR_PTR(err);
> +		goto out;
> +	}
> +
> +	/* walk list locked for safe compare of ino field */
> +	spin_lock(&master_parents_lock);
> +	list_for_each_entry(p, &master_parents, mlist) {
> +		if (p->ino != nd.dentry->d_inode->i_ino ||
> +		    p->flags & AUDIT_PARENT_DEL)
> +			continue;
> +
> +		spin_unlock(&master_parents_lock);
> +		path_release(&nd);
> +		parent = p;
> +		goto out;
> +	}
> +	ino = nd.dentry->d_inode->i_ino;
> +	spin_unlock(&master_parents_lock);
> +	path_release(&nd);
> +
> +	/* Initialize parent with this inode #; the registration thread will
> +	 * catch any changes. */
> +	parent = audit_init_parent(path, ino);
> +	if (unlikely(IS_ERR(parent)))
> +		goto out;
> +
> +	spin_lock(&master_parents_lock);
> +	list_add(&parent->mlist, &master_parents);
> +	spin_unlock(&master_parents_lock);
> +
> +out:
> +	return parent;
> +}
> +
> +/* Find a matching watch entry, or add this one.
> + * Caller must hold exit filterlist lock. */
> +static inline int audit_add_watch(struct audit_krule *krule)
> +{
> +	struct audit_parent *parent;
> +	struct audit_watch *w, *watch = krule->watch;
> +	struct nameidata nd;
> +
> +	parent = audit_find_parent(watch->path);
> +	if (IS_ERR(parent))
> +		return PTR_ERR(parent);
> +
> +	list_for_each_entry(w, &parent->watches, wlist) {
> +		if (audit_compare_watch(watch, w))
> +			continue;
> +
> +		audit_put_watch(watch); /* krule's ref */
> +		audit_put_watch(watch); /* destroy */
> +
> +		audit_get_watch(w);
> +		krule->watch = watch = w;
> +		goto add_rule;
> +	}
> +
> +	audit_get_parent(parent);
> +	watch->parent = parent;
> +	list_add(&watch->wlist, &parent->watches);
> +
> +add_rule:
> +	list_add(&krule->rlist, &watch->rules);
> +
> +	if (path_lookup(watch->path, 0, &nd) == 0)
> +		audit_update_rule(krule, AUDIT_WATCH,
> +				  nd.dentry->d_inode->i_ino);
> +	path_release(&nd);
> +	return 0;
> +}
> +
>  /* Add rule to given filterlist if not a duplicate.  Protected by
>   * audit_netlink_sem. */
>  static inline int audit_add_rule(struct audit_entry *entry,
> -				  struct list_head *list)
> +				 struct audit_flist *flist)
>  {
>  	struct audit_entry *e;
> +	int err;
> +
> +	/* The *_rcu iterator is needed to protect from filterlist
> +	 * updates or removals. */
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(e, &flist->head, list) {
> +		if (e->flags & AUDIT_ENTRY_DEL)
> +			continue;
> +		if (!audit_compare_rule(&entry->rule, &e->rule)) {
> +			err = -EEXIST;
> +			rcu_read_unlock();
> +			goto error;
> +		}
> +	}
> +	rcu_read_unlock();
> +
> +	spin_lock(&flist->lock);
> +	entry->flags |= AUDIT_ENTRY_ADD;
>  
> -	/* Do not use the _rcu iterator here, since this is the only
> -	 * addition routine. */
> -	list_for_each_entry(e, list, list) {
> -		if (!audit_compare_rule(&entry->rule, &e->rule))
> -			return -EEXIST;
> +	if (entry->rule.watch) {
> +		err = audit_add_watch(&entry->rule);
> +		if (err)
> +			goto error;
>  	}
>  
>  	if (entry->rule.flags & AUDIT_FILTER_PREPEND) {
> -		list_add_rcu(&entry->list, list);
> +		list_add_rcu(&entry->list, &flist->head);
>  	} else {
> -		list_add_tail_rcu(&entry->list, list);
> +		list_add_tail_rcu(&entry->list, &flist->head);
>  	}
>  
> +	entry->flags &= ~AUDIT_ENTRY_ADD;
> +	spin_unlock(&flist->lock);
> +

I guess I should look over this more closely... not getting the use of flist->lock.

[..]
>  	return 0;
> +
> +error:
> +	if (entry->rule.watch)
> +		audit_put_watch(entry->rule.watch);
> +	return err;
> +}
> +
> +/* Remove given krule from its associated watch's rules list and clean up any
> + * last instances of associated watch and parent.
> + * Caller must hold exit filterlist lock */
> +static inline void audit_remove_watch(struct audit_krule *krule)
> +{
> +	struct audit_watch *watch = krule->watch;
> +	struct audit_parent *parent = watch->parent;
> +	struct task_struct *task;
> +
> +	list_del(&krule->rlist);
> +	if (list_empty(&watch->rules)) {
> +		list_del(&watch->wlist);
> +		audit_put_watch(watch);
> +
> +		if (list_empty(&parent->watches)) {
> +			/* This flag only read when user adds a watch,
> +			 * which is prevented by audit_netlink_sem. */
> +			parent->flags |= AUDIT_PARENT_DEL;
> +
> +			/* Spawn a thread to unregister the parent's inotify
> +			 * watch without the filterlist spinlock. */
> +			audit_get_parent(parent);
> +			task = kthread_run(audit_inotify_unregister, parent,
> +					   "audit_inotify_unregister");
> +			if (IS_ERR(task))
> +				printk(KERN_ERR
> +			"%s:%d: unable to remove inotify watch for inode %lu\n",
> +				__FILE__, __LINE__, parent->ino);
> +		}
> +	}
>  }
>  
>  /* Remove an existing rule from filterlist.  Protected by
>   * audit_netlink_sem. */
>  static inline int audit_del_rule(struct audit_entry *entry,
> -				 struct list_head *list)
> +				 struct audit_flist *flist)
>  {
>  	struct audit_entry  *e;
> +	int ret = 0;
>  
> -	/* Do not use the _rcu iterator here, since this is the only
> -	 * deletion routine. */
> -	list_for_each_entry(e, list, list) {
> -		if (!audit_compare_rule(&entry->rule, &e->rule)) {
> -			list_del_rcu(&e->list);
> -			call_rcu(&e->rcu, audit_free_rule_rcu);
> -			return 0;
> +	spin_lock(&flist->lock);
> +	list_for_each_entry(e, &flist->head, list) {
> +		if (e->flags & AUDIT_ENTRY_DEL ||
> +		    audit_compare_rule(&entry->rule, &e->rule))
> +			continue;
> +
> +		if (e->rule.watch) {
> +			audit_remove_watch(&e->rule);
> +			audit_put_watch(entry->rule.watch);
>  		}
> +
> +		list_del_rcu(&e->list);
> +		e->flags |= AUDIT_ENTRY_DEL;
> +		call_rcu(&e->rcu, audit_free_rule_rcu);
> +		spin_unlock(&flist->lock);
> +
> +		return ret;
>  	}
> +	spin_unlock(&flist->lock);
> +	if (entry->rule.watch)
> +		audit_put_watch(entry->rule.watch);
>  	return -ENOENT;		/* No matching rule */
>  }
>  
> @@ -379,10 +859,12 @@ static int audit_list(void *_dest)
>  
>  	down(&audit_netlink_sem);
>  
> -	/* The *_rcu iterators not needed here because we are
> -	   always called with audit_netlink_sem held. */
> +	/* The *_rcu iterator is needed to protect from filesystem
> +	 * updates or removals. */
>  	for (i=0; i<AUDIT_NR_FILTERS; i++) {
> -		list_for_each_entry(entry, &audit_filter_list[i], list) {
> +		rcu_read_lock();
> +		list_for_each_entry_rcu(entry, &audit_filter_list[i].head,
> +					list) {
>  			struct audit_rule *rule;
>  
>  			rule = audit_krule_to_rule(&entry->rule);
> @@ -392,6 +874,7 @@ static int audit_list(void *_dest)
>  					 rule, sizeof(*rule));
>  			kfree(rule);
>  		}
> +		rcu_read_unlock();
>  	}
>  	audit_send_reply(pid, seq, AUDIT_LIST, 1, 1, NULL, 0);
>  	
> @@ -413,19 +896,21 @@ static int audit_list_rules(void *_dest)
>  
>  	down(&audit_netlink_sem);
>  
> -	/* The *_rcu iterators not needed here because we are
> -	   always called with audit_netlink_sem held. */
> +	/* The *_rcu iterator is needed to protect from filesystem
> +	 * updates or removals. */
>  	for (i=0; i<AUDIT_NR_FILTERS; i++) {
> -		list_for_each_entry(e, &audit_filter_list[i], list) {
> +		rcu_read_lock();
> +		list_for_each_entry_rcu(e, &audit_filter_list[i].head, list) {
>  			struct audit_rule_data *data;
>  
>  			data = audit_krule_to_data(&e->rule);
>  			if (unlikely(!data))
>  				break;
>  			audit_send_reply(pid, seq, AUDIT_LIST_RULES, 0, 1,
> -					 data, sizeof(*data));
> +					 data, sizeof(*data) + data->buflen);
>  			kfree(data);
>  		}
> +		rcu_read_unlock();
>  	}
>  	audit_send_reply(pid, seq, AUDIT_LIST_RULES, 1, 1, NULL, 0);
>  
> @@ -516,6 +1001,58 @@ int audit_receive_filter(int type, int p
>  	return err;
>  }
>  
> +/* Update inode numbers in audit rules based on filesystem event. */
> +static inline void audit_update_ino(struct audit_parent *parent,
> +				    const char *dname, u32 ino)
> +{
> +	struct audit_watch *w;
> +	struct audit_krule *r, *next;
> +	struct audit_flist *flist = &audit_filter_list[AUDIT_FILTER_EXIT];
> +	struct audit_buffer *ab;
> +
> +	spin_lock(&flist->lock);
> +	list_for_each_entry(w, &parent->watches, wlist) {
> +		if (audit_compare_dname_path(dname, w->path))
> +			continue;
> +
> +		list_for_each_entry_safe(r, next, &w->rules, rlist)
> +			audit_update_rule(r, AUDIT_WATCH, ino);
> +
> +		ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_CONFIG_CHANGE);
> +		audit_log_format(ab, "audit updated rules specifying watch=");
> +		audit_log_untrustedstring(ab, w->path);
> +		audit_log_format(ab, " with ino=%u\n", ino);
> +		audit_log_end(ab);
> +		break;
> +	}
> +	spin_unlock(&flist->lock);
> +}
> +
> +/**
> + * audit_handle_ievent - handler for Inotify events
> + * @event: information about the event
> + * @dname: dentry name associated with event
> + * @inode: inode associated with event
> + * @ptr: kernel's version of a watch descriptor
> + */
> +void audit_handle_ievent(struct inotify_event *event, const char *dname,
> +			  struct inode *inode, void *ptr)
> +{
> +	struct audit_parent *parent = (struct audit_parent *)ptr;
> +
> +	if (event->mask & (IN_CREATE|IN_MOVED_TO) && inode)
> +		audit_update_ino(parent, dname, (unsigned int)inode->i_ino);
> +	else if (event->mask & (IN_DELETE|IN_MOVED_FROM))
> +		audit_update_ino(parent, dname, (unsigned int)-1);
> +	/* Note: Inotify doesn't remove the watch for the IN_MOVE_SELF event.
> +	 * Work around this by leaving the parent around with an empty
> +	 * watchlist.  It will be re-used if new watches are added. */
> +	else if (event->mask & (AUDIT_IN_SELF))
> +		audit_remove_parent_watches(parent);
> +	else if (event->mask & IN_IGNORED)
> +		audit_remove_parent(parent);
> +}
> +
>  int audit_comparator(const u32 left, const u32 op, const u32 right)
>  {
>  	switch (op) {
> @@ -536,7 +1073,39 @@ int audit_comparator(const u32 left, con
>  	}
>  }
>  
> +/* 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?

> +	p = p - dlen + 1;
> +	if (p < path)
> +		return 1;
> +	else if (p > path) {
> +		if (*--p != '/')
> +			return 1;
> +		else
> +			p++;
> +	}
> +
> +	return strncmp(p, dname, dlen);
> +}
>  
>  static int audit_filter_user_rules(struct netlink_skb_parms *cb,
>  				   struct audit_krule *rule,
> @@ -581,7 +1150,8 @@ int audit_filter_user(struct netlink_skb
>  	int ret = 1;
>  
>  	rcu_read_lock();
> -	list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_USER], list) {
> +	list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_USER].head,
> +				list) {
>  		if (audit_filter_user_rules(cb, &e->rule, &state)) {
>  			if (state == AUDIT_DISABLED)
>  				ret = 0;
> @@ -599,10 +1169,10 @@ int audit_filter_type(int type)
>  	int result = 0;
>  	
>  	rcu_read_lock();
> -	if (list_empty(&audit_filter_list[AUDIT_FILTER_TYPE]))
> +	if (list_empty(&audit_filter_list[AUDIT_FILTER_TYPE].head))
>  		goto unlock_and_return;
>  
> -	list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_TYPE],
> +	list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_TYPE].head,
>  				list) {
>  		int i;
>  		for (i = 0; i < e->rule.field_count; i++) {
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 8ff51b3..4626c35 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -60,7 +60,7 @@
>  
>  #include "audit.h"
>  
> -extern struct list_head audit_filter_list[];
> +extern struct audit_flist audit_filter_list[];
>  
>  /* No syscall auditing will take place unless audit_enabled != 0. */
>  extern int audit_enabled;
> @@ -241,7 +241,8 @@ static int audit_filter_rules(struct tas
>  			}
>  			break;
>  		case AUDIT_INODE:
> -			if (ctx) {
> +		case AUDIT_WATCH:
> +			if (ctx && f->val != (unsigned int)-1) {
>  				for (j = 0; j < ctx->name_count; j++) {
>  					if (audit_comparator(ctx->names[j].ino, f->op, f->val) ||
>  					    audit_comparator(ctx->names[j].pino, f->op, f->val)) {
> @@ -286,7 +287,8 @@ static enum audit_state audit_filter_tas
>  	enum audit_state   state;
>  
>  	rcu_read_lock();
> -	list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_TASK], list) {
> +	list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_TASK].head,
> +				list) {
>  		if (audit_filter_rules(tsk, &e->rule, NULL, &state)) {
>  			rcu_read_unlock();
>  			return state;
> @@ -342,7 +344,7 @@ static inline struct audit_context *audi
>  
>  	if (context->in_syscall && !context->auditable) {
>  		enum audit_state state;
> -		state = audit_filter_syscall(tsk, context, &audit_filter_list[AUDIT_FILTER_EXIT]);
> +		state = audit_filter_syscall(tsk, context, &audit_filter_list[AUDIT_FILTER_EXIT].head);
>  		if (state == AUDIT_RECORD_CONTEXT)
>  			context->auditable = 1;
>  	}
> @@ -789,7 +791,7 @@ void audit_syscall_entry(struct task_str
>  
>  	state = context->state;
>  	if (state == AUDIT_SETUP_CONTEXT || state == AUDIT_BUILD_CONTEXT)
> -		state = audit_filter_syscall(tsk, context, &audit_filter_list[AUDIT_FILTER_ENTRY]);
> +		state = audit_filter_syscall(tsk, context, &audit_filter_list[AUDIT_FILTER_ENTRY].head);
>  	if (likely(state == AUDIT_DISABLED))
>  		return;
>  
> @@ -1033,37 +1035,20 @@ void __audit_inode_child(const char *dna
>  		return;
>  
>  	/* determine matching parent */
> -	if (dname)
> -		for (idx = 0; idx < context->name_count; idx++)
> -			if (context->names[idx].pino == pino) {
> -				const char *n;
> -				const char *name = context->names[idx].name;
> -				int dlen = strlen(dname);
> -				int nlen = name ? strlen(name) : 0;
> -
> -				if (nlen < dlen)
> -					continue;
> -				
> -				/* disregard trailing slashes */
> -				n = name + nlen - 1;
> -				while ((*n == '/') && (n > name))
> -					n--;
> -
> -				/* find last path component */
> -				n = n - dlen + 1;
> -				if (n < name)
> -					continue;
> -				else if (n > name) {
> -					if (*--n != '/')
> -						continue;
> -					else
> -						n++;
> -				}
> +	if (!dname)
> +		goto no_match;
> +	for (idx = 0; idx < context->name_count; idx++)
> +		if (context->names[idx].pino == pino) {
> +			const char *name = context->names[idx].name;
>  
> -				if (strncmp(n, dname, dlen) == 0)
> -					goto update_context;
> -			}
> +			if (!name)
> +				continue;
> +
> +			if (audit_compare_dname_path(dname, name) == 0)
> +				goto update_context;
> +		}
>  
> +no_match:
>  	/* catch-all in case match not found */
>  	idx = context->name_count++;
>  	context->names[idx].name  = NULL;
> 
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]