Re: [PATCH] Audit filter rule operators (2/2)
On 10/24/05, Steve Grubb <sgrubb(a)redhat.com> wrote:
On Friday 21 October 2005 19:24, Dustin Kirkland wrote:
> This patch defines the bitmask values of each of the 6 comparators (and
> includes a nice documentation chart explaning how they were chosen).
We need to go ahead and take the next 2 upper bits in the same patch and save
those for future use. For now, if those bits are set, the kernel should
reject the rule. To support this, we also need some code added to
audit_add_rule to check that the operators is something the kernel
understands.
I'm curious why we should seize these bits now? Why the magic number
'2'? Isn't it implied that the next N upper bits will be used for the
next agreed-upon useful purpose? What is the point in rejecting the
rule if those bits are set? The user should understand that if they're
running a kernel that doesn't understand the operations being passed to
it, it won't be able to do anything about it. What about the next two
bits beyond that? What if those are set? Do we just ignore them? For
that matter should we just split the u32 in half and reserve the upper
16 for special flags, and leave the lower 16 for constant enumerations?
I'm not against this, Steve. I'm just trying to understand the goal.
> I didn't add audit_comparator() to audit.h... Should I?
Might this be
> used elsewhere in the audit system?
Not unless you use it somewhere. Keep it local until the need arise to prevent
name collisions.
Thanks, I was unclear on this.
> diff -urpbBN 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.c2005-10-19 09:40:29.000000000 -0500 +++
> linux-2.6.14-rc4-audit_ops/kernel/auditsc.c2005-10-21 18:08:32.000000000
> -0500 @@ -385,6 +385,36 @@ int audit_receive_filter(int type, int p
> return err;
> }
>
> +static int audit_comparator(const u32 left, const u32 operator, const u32
> right) +{
<snip>
> +if ( operator & AUDIT_NEGATE )
> +return !rc;
> +else
> +return rc;
> +}
Does this make sense? What does !< mean? I think AUDIT_NEGATE only makes sense
in relation to AUDIT_EQUAL. It should be moved to that case if not eliminated
outright.
Actually, I think the use of AUDIT_NEGATE should be deprecated all
together. As of this patch, we have two separate operators, one for "="
and one for "!=". AUDIT_NEGATE should never be used in future versions
of audit userspace.
On the other hand, we have to remain backward compatible, in the case
that the user has this patched kernel, but older audit userspace. In
that case, there was an implicit "=", and the AUDIT_NEGATE bit simply
negated the implied "=" when on.
Here's the reasoning I used for the code I previously submitted...
There is no longer an implicit "=", but instead a total of 6 operators
(including "=" and "!=" as separate operations).
I couldn't commandeer the existing AUDIT_NEGATE bit, so how best should
I deal with it logically and remain backward compatible?
It made the most sense to me to have the AUDIT_NEGATE bit simply negate
whatever the rest of the logic determined. In the one case where the
operation is "=", this was make it "!=", which preserves previous
functionality. In the rest of the cases, the table would read something
like this:
~(=) is !=
~(!=) is =
~(>) is <=
~(<) is >=
~(>=) is <
~(<=) is >
Which isn't terribly useful and in fact more confusing if you wrote code
in audit userspace to represent operators in such a way... But
logically, it is valid.
So we have to support AUDIT_NEGATE in so much as to maintain new kernel
compatibility with older audit userspace, though we can decree that the
AUDIT_NEGATE flag is not to be used anymore.
And as for interpreting AUDIT_NEGATE within the kernel, I think as long
as it's doing so in a logically valid manner and maintaining said
backward compatibility, the code should be acceptable.
:-Dustin
Attachments:
- signature.asc (application/pgp-signature — 189 bytes)