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

Re: [PATCH 2/2] audit string fields interface + consumer



Hi Amy,

First pass comments.

-tim


On Wednesday 11 January 2006 13:04, Amy Griffis wrote:
> Add AUDIT_WATCH field type and associated helpers.
> 
> Signed-off-by: Amy Griffis <amy griffis hp com>
> 
> ---
> 
>  include/linux/audit.h |    1 
>  kernel/audit.h        |    8 +++
>  kernel/auditfilter.c  |  122 +++++++++++++++++++++++++++++++++++++++++++++----
>  kernel/auditsc.c      |    3 +
>  4 files changed, 123 insertions(+), 11 deletions(-)
> 
> d7ade2dd92b0ff7a3c6488b068f77089c9952d93
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index c208554..d76fa58 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -148,6 +148,7 @@
>  #define AUDIT_INODE	102
>  #define AUDIT_EXIT	103
>  #define AUDIT_SUCCESS   104	/* exit >= 0; value ignored */
> +#define AUDIT_WATCH	105
>  
>  #define AUDIT_ARG0      200
>  #define AUDIT_ARG1      (AUDIT_ARG0+1)
> diff --git a/kernel/audit.h b/kernel/audit.h
> index f3b2a00..cc979e9 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -52,6 +52,12 @@ enum audit_state {
>  };
>  
>  /* Rule lists */
> +struct audit_watch {
> +	char			*path; /* watch insertion path */
> +	struct list_head	mlist; /* entry in master_watchlist */
> +	struct list_head	rules; /* associated rules */
> +};
> +
>  struct audit_field {
>  	u32			type;
>  	u32			val;
> @@ -67,6 +73,8 @@ struct audit_krule {
>  	u32			buflen; /* for data alloc on list rules */
>  	u32			field_count;
>  	struct audit_field	fields[AUDIT_MAX_FIELDS];
> +	struct audit_watch	*watch; /* associated watch */
> +	struct list_head	rlist; /* entry in audit_watch.rules list */
>  };
>  
>  struct audit_entry {
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 9c8865e..8ea0a14 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -22,6 +22,8 @@
>  #include <linux/kernel.h>
>  #include <linux/audit.h>
>  #include <linux/kthread.h>
> +#include <linux/fs.h>
> +#include <linux/namei.h>
>  #include <linux/netlink.h>
>  #include "audit.h"
>  
> @@ -40,6 +42,8 @@ struct list_head audit_filter_list[AUDIT
>  #endif
>  };
>  
> +static LIST_HEAD(master_watchlist);
> +
>  /* Unpack a filter field's string representation from user-space
>   * buffer. */
>  static char *audit_unpack_string(void **bufp, size_t *remain, size_t len)
> @@ -67,6 +71,34 @@ 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)
> +{
> +	int err;
> +	struct audit_field *f = &krule->fields[fidx];
> +	struct nameidata nd;
> +	struct audit_watch *watch;
> +
> +	err = -EINVAL;
> +	if (path[0] != '/' || krule->listnr != AUDIT_FILTER_EXIT ||
> +	    f->op & ~AUDIT_EQUAL)
> +		return err;
> +

What about path[&krule->values[fidx] - 1] (I going on memory that that's where len is stored)] != '/'.
The reason I bring this is up is that with my implementation, '/tmp/foo' and /tmp/foo/' were treated
differently, but if '/tmp/foo' is a directory, then technically they translate to the same thing.

[..]
> +	if (path_lookup(path, 0, &nd) == 0)
> +		f->val = nd.dentry->d_inode->i_ino;
> +	else
> +		f->val = (unsigned int)-1;
> +

path_release(&nd)?

> +	err = -ENOMEM;
> +	watch = kmalloc(sizeof(*watch), GFP_KERNEL);
> +	if (!watch)
> +		return err;
> +	watch->path = path;
> +	krule->watch = watch;
> +
> +	return 0;
> +}
> +
>  /* Common user-space to kernel rule translation. */
>  static inline struct audit_entry *audit_to_entry_common(struct audit_rule *rule)
>  {
> @@ -161,8 +193,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); */
> -	int i;
> +	size_t remain = datasz - sizeof(struct audit_rule_data);
> +	int i, len;
> +	char *path;
>  
>  	entry = audit_to_entry_common((struct audit_rule *)data);
>  	if (IS_ERR(entry))
> @@ -181,7 +214,19 @@ static struct audit_entry *audit_data_to
>  		f->op = data->fieldflags[i] & AUDIT_OPERATORS;
>  		f->type = data->fields[i];
>  		switch(f->type) {
> -		/* call type-specific conversion routines here */
> +		case AUDIT_WATCH:
> +			len = data->values[i];
> +			path = audit_unpack_string(&bufp, &remain, len);
> +			if (IS_ERR(path))
> +				goto exit_free;
> +			entry->rule.buflen += len;
> +
> +			err = audit_to_watch(path, &entry->rule, i);
> +			if (err) {
> +				kfree(path);
> +				goto exit_free;
> +			}
> +			break;
>  		default:
>  			f->val = data->values[i];
>  		}
> @@ -259,7 +304,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;
>  		}
> @@ -269,6 +317,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);
> +}
> +

Does device matter?  For instance,

/tmp/foo on /dev/hda3 might be important, but /tmp/foo on /dev/hda4 uninteresting.

Or for that matter, they might both be interesting, but for different reasons.  I know
that's a little far fetched, but something to consider, I suppose.  This could also
probably apply for namespace... and I'd imagine when this goes to linux-fsdevel
these types of limitations will need to be answered for.

[..]
>  /* 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)
> @@ -287,7 +341,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;
> @@ -301,6 +358,38 @@ static int audit_compare_rule(struct aud
>  	return 0;
>  }
>  
> +static inline void audit_free_watch(struct audit_watch *watch)
> +{
> +	kfree(watch->path);
> +	kfree(watch);
> +}
> +
> +static inline void audit_free_rule(struct rcu_head *head)
> +{
> +	struct audit_entry *e = container_of(head, struct audit_entry, rcu);
> +	kfree(e);
> +}
> +
> +/* Attach krule's watch to master_watchlist, using existing watches
> + * when possible. */
> +static inline void audit_add_watch(struct audit_krule *krule)
> +{
> +	struct audit_watch *w;
> +
> +	list_for_each_entry(w, &master_watchlist, mlist) {
> +		if (strcmp(w->path, krule->watch->path) != 0)
> +			continue;

audit_compare_watch()?

[..]
> +
> +		audit_free_watch(krule->watch);
> +		krule->watch = w;
> +		list_add(&krule->rlist, &w->rules);
> +		return;
> +	}
> +	INIT_LIST_HEAD(&krule->watch->rules);
> +	list_add(&krule->rlist, &krule->watch->rules);
> +	list_add(&krule->watch->mlist, &master_watchlist);
> +}
> +
>  /* Add rule to given filterlist if not a duplicate.  Protected by
>   * audit_netlink_sem. */
>  static inline int audit_add_rule(struct audit_entry *entry,
> @@ -315,6 +404,7 @@ static inline int audit_add_rule(struct 
>  			return -EEXIST;
>  	}
>  
> +	audit_add_watch(&entry->rule);
>  	if (entry->rule.flags & AUDIT_FILTER_PREPEND) {
>  		list_add_rcu(&entry->list, list);
>  	} else {
> @@ -324,10 +414,18 @@ static inline int audit_add_rule(struct 
>  	return 0;
>  }
>  
> -static inline void audit_free_rule(struct rcu_head *head)
> +/* Detach watch from krule, freeing if it has no associated rules. */
> +static inline void audit_remove_watch(struct audit_krule *krule)
>  {
> -	struct audit_entry *e = container_of(head, struct audit_entry, rcu);
> -	kfree(e);
> +	struct audit_watch *watch = krule->watch;
> +
> +	list_del(&krule->rlist);
> +	krule->watch = NULL;
> +
> +	if (list_empty(&watch->rules)) {
> +		list_del(&watch->mlist);
> +		audit_free_watch(watch);
> +	}
>  }
>  
>  /* Remove an existing rule from filterlist.  Protected by
> @@ -342,6 +440,7 @@ static inline int audit_del_rule(struct 
>  	list_for_each_entry(e, list, list) {
>  		if (!audit_compare_rule(&entry->rule, &e->rule)) {
>  			list_del_rcu(&e->list);
> +			audit_remove_watch(&e->rule);
>  			call_rcu(&e->rcu, audit_free_rule);
>  			return 0;
>  		}
> @@ -408,7 +507,7 @@ static int audit_list_rules(void *_dest)
>  			if (!data)
>  				break;
>  			audit_send_reply(pid, seq, AUDIT_LIST_RULES, 0, 1,
> -					 data, sizeof(*data));
> +					 data, sizeof(*data) + data->buflen);
>  			kfree(data);
>  		}
>  	}
> @@ -475,8 +574,10 @@ int audit_receive_filter(int type, int p
>  		if (!err)
>  			audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
>  				  "auid=%u added an audit rule\n", loginuid);
> -		else
> +		else {
> +			audit_free_watch(entry->rule.watch);
>  			kfree(entry);
> +		}
>  		break;
>  	case AUDIT_DEL:
>  	case AUDIT_DEL_RULE:
> @@ -492,6 +593,7 @@ int audit_receive_filter(int type, int p
>  		if (!err)
>  			audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
>  				  "auid=%u removed an audit rule\n", loginuid);
> +		audit_free_watch(entry->rule.watch);
>  		kfree(entry);
>  		break;
>  	default:
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index e4f7096..8e98b65 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -240,7 +240,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)) {


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