Darrel Goeddel initiated a discussion on IRC regarding the possibility
of audit_comparator() returning -EINVAL signaling an invalid operator.
It is possible when creating the rule to assure that the operator is one
of the 6 sane values. Here's a snip from include/linux/audit.h Note
that 0 (nonsense) and 7 (all operators) are not valid values for an
operator.
...
#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)
...
Furthermore, prior to adding these extended operators, flagging the
AUDIT_NEGATE bit implied !=, and otherwise == was assumed.
The following code forces the operator to be != if the AUDIT_NEGATE bit
was flipped on. And if no operator was specified, == is assumed. The
only invalid condition is if the AUDIT_NEGATE bit is off and all of the
AUDIT_EQUAL, AUDIT_LESS_THAN, and AUDIT_GREATER_THAN bits are
on--clearly a nonsensical operator.
Now that this is handled at rule insertion time, the default -EINVAL
return of audit_comparator() is eliminated such that the function can
only return 1 or 0.
If this is acceptable, let's get this applied to the current tree.
:-Dustin
--
diff -urpN a/kernel/auditfilter.c b/kernel/auditfilter.c
--- a/kernel/auditfilter.c 2006-02-16 11:57:17.000000000 -0600
+++ b/kernel/auditfilter.c 2006-02-16 13:30:33.000000000 -0600
@@ -139,11 +139,17 @@ static struct audit_entry *audit_rule_to
f->val = rule->values[i];
entry->rule.vers_ops = (f->op & AUDIT_OPERATORS) ? 2 : 1;
+
+ /* Support for legacy operators where
+ * AUDIT_NEGATE bit signifies != and otherwise assumes == */
if (f->op & AUDIT_NEGATE)
- f->op |= AUDIT_NOT_EQUAL;
- else if (!(f->op & AUDIT_OPERATORS))
- f->op |= AUDIT_EQUAL;
- f->op &= ~AUDIT_NEGATE;
+ f->op = AUDIT_NOT_EQUAL;
+ else if (!f->op)
+ f->op = AUDIT_EQUAL;
+ else if (f->op == AUDIT_OPERATORS) {
+ err = -EINVAL;
+ goto exit_free;
+ }
}
exit_nofree:
@@ -537,8 +543,6 @@ int audit_comparator(const u32 left, con
return (left > right);
case AUDIT_GREATER_THAN_OR_EQUAL:
return (left >= right);
- default:
- return -EINVAL;
}
}
Show replies by date
Steve Grubb mentioned that BUG() should be called in the
should-be-possible situation of an invalid op to audit_comparator().
I added this, plus a "return 0" that should never be" called to assuage
the compiler worrying about this function completing without ever having
returned an int.
Updated patch below.
:-Dustin
--- a/kernel/auditfilter.c 2006-02-16 11:57:17.000000000 -0600
+++ b/kernel/auditfilter.c 2006-02-16 23:28:23.000000000 -0600
@@ -139,11 +139,17 @@ static struct audit_entry *audit_rule_to
f->val = rule->values[i];
entry->rule.vers_ops = (f->op & AUDIT_OPERATORS) ? 2 : 1;
+
+ /* Legacy support for simple operator support
+ * where AUDIT_NEGATE bit signified != and otherwise assumed == */
if (f->op & AUDIT_NEGATE)
- f->op |= AUDIT_NOT_EQUAL;
- else if (!(f->op & AUDIT_OPERATORS))
- f->op |= AUDIT_EQUAL;
- f->op &= ~AUDIT_NEGATE;
+ f->op = AUDIT_NOT_EQUAL;
+ else if (!f->op)
+ f->op = AUDIT_EQUAL;
+ else if (f->op == AUDIT_OPERATORS) {
+ err = -EINVAL;
+ goto exit_free;
+ }
}
exit_nofree:
@@ -537,9 +543,10 @@ int audit_comparator(const u32 left, con
return (left > right);
case AUDIT_GREATER_THAN_OR_EQUAL:
return (left >= right);
- default:
- return -EINVAL;
}
+ /* should NEVER get here; op checked on rule insertion */
+ BUG();
+ return 0;
}
int audit_str_comparator(const char *left, const u32 op, const char *right)