2:34 p.m.
On Thu, 2005-12-15 at 10:40 -0500, Amy Griffis wrote:
1) struct audit_rule_xprt
Introducing a new data structure for specifying audit rules via
netlink provides a good opportunity to revisit the data structure
design and determine if we want to make any other changes, e.g.
adding a structure version field, reserving fields, etc. At
present, I've only added the empty buf[] array.
nit: It also adds a buflen integer field.
You touch on this a bit later... Ideally, I think it would be nice not
to bitmask into the upper bits of each field[] entry. I would prefer
another integer array fieldflags[] where things such as the
audit_operator and whatever else might live. That would give us a full
32 bits to mask against per field, and not cut into the total bits
available for field[] values.
2) struct audit_krule
The kernel structure defining an audit rule isn't keeping enough
information about what it received from userspace.
- the kernel currently converts the upper bits of a field[]
element to its own representation; when the rule is listed back
to userspace, the bits are not converted back to the way in
which they were originally specified
See my comment above. I'd prefer field[] being just a field, and having
flags associated with each field in a separate array.
- the difference between an inode-based and a path-based filter
is
not always discernible, as it needs to be (e.g. kernel currently
won't allow a rule to be added for an inode # that can be
resolved from an existing path-based filter)
Might be a good thing to note by flipping a bit in the aforementioned
potentially new fieldflags[] location.
- might not be an issue now, but should probably track which
type
or version of structure was used to add a given rule
Hard to see, but I think a version field could be useful as the utility
of the audit system grows.
- the operator bits consistently need to be masked out; they
should just be a separate field in kernel
Agreed. See comments above.
4) in general
Perhaps most importantly, this patch mixes the interface changes
required to specify audit rules with string fields with some of
the initial filesystem audit functionality. I think these pieces
should be split into separate patches to allow a more generalized
approach.
Yep, good call. It would be nice if the new struct audit_rule_xprt plus
the functions required to receive the netlink messages and populate the
kernel structure were a patch of it's own. I would absolutely begin
testing and developing on top of that as soon as such a patch is
available.
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -182,7 +182,33 @@ struct audit_context {
#endif
};
- /* Public API */
+/* Audit Filters */
+
+/* kernel's representation of a watched filesystem location */
+struct audit_watch {
+ char *path; /* watch insertion path */
+ struct list_head mlist; /* entry in master_watchlist */
+ struct list_head rules; /* associated rules*/
+};
+
+/* kernel's internal filter rule representation */
+struct audit_krule {
+ u32 flags;
+ u32 action;
+ u32 field_count;
+ u32 mask[AUDIT_BITMASK_SIZE];
+ u32 fields[AUDIT_MAX_FIELDS];
+ u32 values[AUDIT_MAX_FIELDS];
+ struct audit_watch *watch; /* associated watch */
+ struct list_head rlist; /* entry in audit_watch.rules list */
+};
+
+struct audit_entry {
+ struct list_head list; /* entry in audit_filter_list */
+ struct rcu_head rcu;
+ struct audit_krule rule;
+};
+
/* There are three lists of rules -- one to search at task creation
* time, one to search at syscall entry time, and another to search at
* syscall exit time. */
Not really your problem, but this documentation should probably be
updated, in that there are several more lists of rules now.
@@ -198,101 +224,290 @@ static struct list_head audit_filter_lis
#endif
};
-struct audit_entry {
- struct list_head list;
- struct rcu_head rcu;
- struct audit_rule rule;
-};
+static LIST_HEAD(master_watchlist);
extern int audit_pid;
-/* Copy rule from user-space to kernel-space. Called from
- * audit_add_rule during AUDIT_ADD. */
-static inline int audit_copy_rule(struct audit_rule *d, struct audit_rule *s)
+/* Check to see if two rules are identical. */
+static inline int audit_compare_rule(struct audit_krule *a,
+ struct audit_krule *b,
+ unsigned skip_ino)
{
int i;
- if (s->action != AUDIT_NEVER
- && s->action != AUDIT_POSSIBLE
- && s->action != AUDIT_ALWAYS)
- return -1;
- if (s->field_count < 0 || s->field_count > AUDIT_MAX_FIELDS)
- return -1;
- if ((s->flags & ~AUDIT_FILTER_PREPEND) >= AUDIT_NR_FILTERS)
- return -1;
-
- d->flags = s->flags;
- d->action = s->action;
- d->field_count = s->field_count;
- for (i = 0; i < d->field_count; i++) {
- d->fields[i] = s->fields[i];
- d->values[i] = s->values[i];
+ if (a->flags != b->flags ||
+ a->action != b->action ||
+ a->field_count != b->field_count)
+ return 1;
+
+ /* rules must have same field ordering to match */
+ for (i = 0; i < a->field_count; i++) {
+ /* skip inode comparison with matching paths */
+ if (skip_ino &&
+ (a->fields[i] & ~AUDIT_OPERATORS) == AUDIT_INODE &&
+ (b->fields[i] & ~AUDIT_OPERATORS) == AUDIT_INODE)
+ continue;
I can see where this masking out of AUDIT_OPERATORS could get tedious.
These flags should be in it's own component of the struct.
As for the rest of the patch, it would be much easier to review in
separate pieces--ie, the (1) new rule structure handling code, then (2)
the new patch watching filter.
Thanks so much for posting, Amy!
:-Dustin