On 10/24/05, Steve Grubb <sgrubb(a)redhat.com&gt; wrote: On 10/24/05, Steve Grubb <sgrubb(a)redhat.com&gt; wrote: On 10/25/05, Amy Griffis <amy.griffis(a)hp.com&gt; wrote: Ok, as per our subsequent discussions via email and IRC, I've actually created a bitmask called AUDIT_UNUSED_BITS which is currently in sync with the constants we now have defined. If we start #defining audit message types > 2048, or start extending the operator bits further right, this simply needs to be updated. Then in the audit_add_rule() function, there's a call that checks to make sure that each field value in the new rule is not using unsupported bits. I think this should give the future compatibility check that Steve has been asking for. Which means that the default: return -EINVAL; in the audit_comparator() switch should be dead code, assuming that AUDIT_UNUSED_BITS is in good working order. Also near this rule insertion code, there's a new check for the AUDIT_NEGATE bit being set. If it is, I unset it and make the operator AUDIT_NOT_EQUAL. And if all of the the operator bits are 0's, then we assume that it's an AUDIT_EQUAL operation. These five lines will be needed for legacy support of older audit userspace that utilized AUDIT_NEGATE and flagged AUDIT_EQUAL in such a manner. On 10/24/05, Steve Grubb <sgrubb(a)redhat.com&gt; wrote: You're absolutely right, Steve. On 10/25/05, Amy Griffis <amy.griffis(a)hp.com&gt; wrote: Phenomenal suggestion, Amy. I've reworked them to look much nicer, I think you'll see. On 10/25/05, Amy Griffis <amy.griffis(a)hp.com&gt; wrote: This is totally true, as well. I've been testing with some rather complicated sets of rules that have in fact proven to operate as expected in range fashion. I meant to remove this part of the patch but I forgot as I was cleansing it before submission. On 10/25/05, Amy Griffis <amy.griffis(a)hp.com&gt; wrote: I fixed all of these. Please check to make sure I've indented them in an acceptable manner. On 10/25/05, Amy Griffis <amy.griffis(a)hp.com&gt; wrote: Ok, so there's been a side thread of discussion among myself, Amy, and Steve where we've created a test program that tests both algorithms and we're seeing almost precisely identical performance of both algorithms across several architectures. It seems that there's a very slight advantage of my switch-based approach on ix86, x86_64, and ppc, though the different is possibly attributable to other performance noise. Amy's if-based algorithm seems to have an advantage on ia64 though we don't yet have an explanation as to why, though her last email seemed to indicated that with compiler optimization at -O2, the results were again nearly identical. Thus, I think that the switch-based approach is easier on the eyes and I'm putting it forward again as my suggestion. Feel free to add your two cents here, and perhaps we should publish the tests as well... -- A couple of other changes... - I've incorporated the use of audit_comparator() in audit_filter_user_rules() as well. - I've also left a bit of explanation in the comment just above the defines of the AUDIT_* stuff--let me know if this is ridiculously elementary for the audience, as I can gladly remove it. Patch follows. :-Dustin diff -urpNbB linux-2.6.14-rc4/include/linux/audit.h linux-2.6.14-rc4-audit_ops/include/linux/audit.h --- linux-2.6.14-rc4/include/linux/audit.h 2005-10-19 09:40:27.000000000 -0500 +++ linux-2.6.14-rc4-audit_ops/include/linux/audit.h 2005-10-26 16:12:42.000000000 -0500 @@ -98,6 +98,13 @@ #define AUDIT_WORD(nr) ((__u32)((nr)/32)) #define AUDIT_BIT(nr) (1 << ((nr) - AUDIT_WORD(nr)*32)) +/* This bitmask is used to validate user input. It represents all bits that + * are currently used in an audit field constant understood by the kernel. + * If you are adding a new #define AUDIT_<whatever>, please ensure that + * AUDIT_UNUSED_BITS is updated if need be. */ +#define AUDIT_UNUSED_BITS 0x0FFFFC00 + + /* Rule fields */ /* These are useful when checking the * task structure at task creation time @@ -130,6 +137,26 @@ #define AUDIT_NEGATE 0x80000000 +/* These are the supported operators. + * 4 2 1 + * = > < + * ------- + * 0 0 0 0 nonsense + * 0 0 1 1 < + * 0 1 0 2 > + * 0 1 1 3 != + * 1 0 0 4 = + * 1 0 1 5 <= + * 1 1 0 6 >= + * 1 1 1 7 all operators + */ +#define AUDIT_LESS_THAN 0x10000000 +#define AUDIT_GREATER_THAN 0x20000000 +#define AUDIT_NOT_EQUAL 0x30000000 +#define AUDIT_EQUAL 0x40000000 +#define AUDIT_LESS_THAN_OR_EQUAL (AUDIT_LESS_THAN|AUDIT_EQUAL) +#define AUDIT_GREATER_THAN_OR_EQUAL (AUDIT_GREATER_THAN|AUDIT_EQUAL) +#define AUDIT_OPERATORS (AUDIT_EQUAL|AUDIT_NOT_EQUAL) /* Status symbols */ /* Mask values */ diff -urpNbB linux-2.6.14-rc4/kernel/auditsc.c linux-2.6.14-rc4-audit_ops/kernel/auditsc.c --- linux-2.6.14-rc4/kernel/auditsc.c 2005-10-19 09:40:29.000000000 -0500 +++ linux-2.6.14-rc4-audit_ops/kernel/auditsc.c 2005-10-26 16:11:10.000000000 -0500 @@ -2,6 +2,7 @@ * Handles all system-call specific auditing features. * * Copyright 2003-2004 Red Hat Inc., Durham, North Carolina. + * Copyright (C) 2005 IBM Corporation * All Rights Reserved. * * This program is free software; you can redistribute it and/or modify @@ -27,6 +28,9 @@ * this file -- see entry.S) is based on a GPL'd patch written by * okir(a)suse.de and Copyright 2003 SuSE Linux AG. * + * The support of additional filter rules compares (>, <, >=, <=) was + * added by Dustin Kirkland <dustin.kirkland(a)us.ibm.com&gt;, 2005. + * */ #include <linux/init.h> @@ -252,6 +256,7 @@ static inline int audit_add_rule(struct struct list_head *list) { struct audit_entry *entry; + int i; /* Do not use the _rcu iterator here, since this is the only * addition routine. */ @@ -261,6 +266,16 @@ static inline int audit_add_rule(struct } } + for (i = 0; i < rule->field_count; i++) { + if (rule->fields[i] & AUDIT_UNUSED_BITS) + return -EINVAL; + if ( rule->fields[i] & AUDIT_NEGATE ) + rule->fields[i] |= AUDIT_NOT_EQUAL; + else if ( (rule->fields[i] & AUDIT_OPERATORS) == 0 ) + rule->fields[i] |= AUDIT_EQUAL; + rule->fields[i] &= (~AUDIT_NEGATE); + } + if (!(entry = kmalloc(sizeof(*entry), GFP_KERNEL))) return -ENOMEM; if (audit_copy_rule(&entry->rule, rule)) { @@ -385,6 +400,27 @@ int audit_receive_filter(int type, int p return err; } +static int audit_comparator(const u32 left, const u32 op, const u32 right) +{ + printk(KERN_ERR "DUSTIN: peforming (%d %x %d)\n", left, op, right); + switch (op) { + case AUDIT_EQUAL: + return (left == right); + case AUDIT_NOT_EQUAL: + return (left != right); + case AUDIT_LESS_THAN: + return (left < right); + case AUDIT_LESS_THAN_OR_EQUAL: + return (left <= right); + case AUDIT_GREATER_THAN: + return (left > right); + case AUDIT_GREATER_THAN_OR_EQUAL: + return (left >= right); + default: + return -EINVAL; + } +} + /* Compare a task_struct with an audit_rule. Return 1 on match, 0 * otherwise. */ static int audit_filter_rules(struct task_struct *tsk, @@ -395,62 +431,71 @@ static int audit_filter_rules(struct tas int i, j; for (i = 0; i < rule->field_count; i++) { - u32 field = rule->fields[i] & ~AUDIT_NEGATE; + u32 field = rule->fields[i] & ~AUDIT_OPERATORS; + u32 op = rule->fields[i] & AUDIT_OPERATORS; u32 value = rule->values[i]; int result = 0; switch (field) { case AUDIT_PID: - result = (tsk->pid == value); + result = audit_comparator(tsk->pid, op, value); break; case AUDIT_UID: - result = (tsk->uid == value); + result = audit_comparator(tsk->uid, op, value); break; case AUDIT_EUID: - result = (tsk->euid == value); + result = audit_comparator(tsk->euid, op, value); break; case AUDIT_SUID: - result = (tsk->suid == value); + result = audit_comparator(tsk->suid, op, value); break; case AUDIT_FSUID: - result = (tsk->fsuid == value); + result = audit_comparator(tsk->fsuid, op, value); break; case AUDIT_GID: - result = (tsk->gid == value); + result = audit_comparator(tsk->gid, op, value); break; case AUDIT_EGID: - result = (tsk->egid == value); + result = audit_comparator(tsk->egid, op, value); break; case AUDIT_SGID: - result = (tsk->sgid == value); + result = audit_comparator(tsk->sgid, op, value); break; case AUDIT_FSGID: - result = (tsk->fsgid == value); + result = audit_comparator(tsk->fsgid, op, value); break; case AUDIT_PERS: - result = (tsk->personality == value); + result = audit_comparator(tsk->personality, op, value); break; case AUDIT_ARCH: if (ctx) - result = (ctx->arch == value); + result = audit_comparator(ctx->arch, op, value); break; case AUDIT_EXIT: if (ctx && ctx->return_valid) - result = (ctx->return_code == value); + result = audit_comparator(ctx->return_code, + op, value); break; case AUDIT_SUCCESS: if (ctx && ctx->return_valid) { if (value) - result = (ctx->return_valid == AUDITSC_SUCCESS); + result = audit_comparator( + ctx->return_valid, + op, AUDITSC_SUCCESS); else - result = (ctx->return_valid == AUDITSC_FAILURE); + result = audit_comparator( + ctx->return_valid, op, + AUDITSC_FAILURE); } break; case AUDIT_DEVMAJOR: if (ctx) { for (j = 0; j < ctx->name_count; j++) { - if (MAJOR(ctx->names[j].dev)==value) { + if ( audit_comparator( + MAJOR(ctx->names[j].dev), + op, value) + ) { ++result; break; } @@ -460,7 +505,10 @@ static int audit_filter_rules(struct tas case AUDIT_DEVMINOR: if (ctx) { for (j = 0; j < ctx->name_count; j++) { - if (MINOR(ctx->names[j].dev)==value) { + if ( audit_comparator( + MINOR(ctx->names[j].dev), + op, value) + ) { ++result; break; } @@ -470,7 +518,9 @@ static int audit_filter_rules(struct tas case AUDIT_INODE: if (ctx) { for (j = 0; j < ctx->name_count; j++) { - if (ctx->names[j].ino == value) { + if ( audit_comparator( + ctx->names[j].ino, op, value) + ) { ++result; break; } @@ -480,19 +530,20 @@ static int audit_filter_rules(struct tas case AUDIT_LOGINUID: result = 0; if (ctx) - result = (ctx->loginuid == value); + result = audit_comparator(ctx->loginuid, op, + value); break; case AUDIT_ARG0: case AUDIT_ARG1: case AUDIT_ARG2: case AUDIT_ARG3: if (ctx) - result = (ctx->argv[field-AUDIT_ARG0]==value); + result = audit_comparator( + ctx->argv[field-AUDIT_ARG0], + op, value); break; } - if (rule->fields[i] & AUDIT_NEGATE) - result = !result; if (!result) return 0; } @@ -563,27 +614,26 @@ static int audit_filter_user_rules(struc int i; for (i = 0; i < rule->field_count; i++) { - u32 field = rule->fields[i] & ~AUDIT_NEGATE; + u32 field = rule->fields[i] & ~AUDIT_OPERATORS; + u32 op = rule->fields[i] & AUDIT_OPERATORS; u32 value = rule->values[i]; int result = 0; switch (field) { case AUDIT_PID: - result = (cb->creds.pid == value); + result = audit_comparator(cb->creds.pid, op, value); break; case AUDIT_UID: - result = (cb->creds.uid == value); + result = audit_comparator(cb->creds.uid, op, value); break; case AUDIT_GID: - result = (cb->creds.gid == value); + result = audit_comparator(cb->creds.gid, op, value); break; case AUDIT_LOGINUID: - result = (cb->loginuid == value); + result = audit_comparator(cb->loginuid, op, value); break; } - if (rule->fields[i] & AUDIT_NEGATE) - result = !result; if (!result) return 0; }