[PATCH 2/2] audit string fields interface + consumer
Timothy R. Chavez
tinytim at us.ibm.com
Thu Jan 12 23:07:04 UTC 2006
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 at 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)) {
More information about the Linux-audit
mailing list