Re: [PATCH 2/2] audit string fields interface + consumer
On Thu, Jan 12, 2006 at 05:07:04PM -0600, Timothy R. Chavez wrote:
First pass comments.
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(a)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
<snip>
> @@ -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.
Right now auditctl trims any trailing slashes. I think that decision
was made to avoid the ambiguity that comes with supporting them. We
could add an additional sanity check in the kernel if you think it's
necessary.
> + if (path_lookup(path, 0, &nd) == 0)
> + f->val = nd.dentry->d_inode->i_ino;
> + else
> + f->val = (unsigned int)-1;
> +
path_release(&nd)?
Yes, thanks.
<snip>
> @@ -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?
I suppose that depends on your definition of "matter". :-)
To date, I believe this audit implementation has ignored device when
checking for existing rules, or when filtering during syscalls. It
does however, include the device information in the audit record.
This is the same whether filtering on inodes or paths. The device
information is stored in struct audit_names in the audit_context.
I think that for now it is sufficient to require the admin to update
their audit rules appropriately when mounting one filesystem over the
top of another one.
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.
I could be wrong, but I don't think fsdevel will care about
limitations we've imposed on audit's behaviour. I think the audit
team is free to define the features of the audit subsystem, and
fsdevel will review for impacts to the core filesystem code.
<snip>
> @@ -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()?
Good catch. I'll fix this.
Thanks for reviewing.
Amy